Fix: [Capacitor] Set overscroll background color from theme #2594

msdewitt · 2024-11-16T20:41:17Z

I added a way to eliminate the ios bounce outside the boundary of the app while adding a scroll feature within the controllable range of the app code instead of the device.

msdewitt · 2024-11-16T20:50:52Z

@raineorshine The difference in puppeteer tests on this pr is caused by turning off the bounce feature of ios and taking inspiration from @ankitkarna99's proposed solution in the issue: Issue 1700.

Originally, it is suggested to set scroll to false, but this caused issues with the webView. Instead, I opted for a solution similar to this, disabling the scroll bounce within the ios webview and setting the scrolling power to within our app itself.

We cannot set the background color dynamically on the ios webview as we do with javascript when a user changes themes, so we have to opt for a solution like this which either hides or limits the user from seeing the webview background behind the app.

With these changes, there is a drawback. By disabling the scroll bounce, it disables the scroll refresh feature which naturally comes with the Ios Webview.

@raineorshine please tell me your thoughts on this issue based on this information.

raineorshine

Thanks for the update! This looks like a promising approach. The overscroll background color now correctly matches the theme background color based on my testing.

We cannot set the background color dynamically on the ios webview as we do with javascript when a user changes themes, so we have to opt for a solution like this which either hides or limits the user from seeing the webview background behind the app.

Got it. That makes sense, and good to know the limitations with the WebView.

With these changes, there is a drawback. By disabling the scroll bounce, it disables the scroll refresh feature which naturally comes with the Ios Webview.

That's fine, we don't use that feature.

The only issue I am seeing is that programmatic scroll functionality is broken. Here are some examples, although there are likely others if window.scrollTo is not working everywhere.

Scroll zone parallax - When the user scrolls, the scroll zone on the right should scroll independently, at a reduced rate.
scrollCursorIntoView - When the cursor moves to a thought that is outside the viewport, it should automatically scroll to the thought. To reproduce, activate New Subthought on a thought with a long list of children that exceeds the height of the viewport. It should scroll down to the new thought.
etc

src/App.css

capacitor.config.ts

msdewitt · 2024-11-18T19:10:43Z

It looks like it's working.

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-18.at.13.10.11.mp4

raineorshine · 2024-11-18T19:38:34Z

It looks like it's working.

Hmmm sorry about that! Maybe my branch was not up-to-date, or I failed to build the Capacitor app correctly. I do see now that the parallax scrolling and scrollCursorIntoVIew are working as expected.

Now what I'm seeing in the XCode simulator is that scroll bounce on overscroll is disabled completely. The scroll stops when it reaches the top. Just to confirm, is that what you have as well?

msdewitt · 2024-11-18T19:54:19Z

@raineorshine Are you talking about where you are scroll to the very bottom and create a new sub-thought?

Like this:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-18.at.13.52.54.mp4

I also thought this was not expected, but when I double-checked if the same behavior happens on main I found that it's exactly the same as my branch.

raineorshine · 2024-11-18T20:01:55Z

I'm referring to the "scroll bounce" or "elastic scroll" behavior that occurs when scrolling past the top of the document.

Here is the behavior on main on iOS PWA:

trim.2994A651-A7B9-4032-900E-6FF8C26DC760.MOV

msdewitt · 2024-11-18T20:10:55Z

@raineorshine yes, that scroll bounce is actually a part of the UIScrollView of the IOS device. It is disabled in IOS in order to prevent the overscroll background from being shown since we can't dynamically alter the color of that (Its not javascript.)

This is only an issue with IOS devices so this fix is here.

Here is a video of the scroll bounce enabled and the UIScrollView background color set to white for visibility:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-18.at.14.10.38.mp4

raineorshine · 2024-11-18T20:52:44Z

I see, thanks. Unfortunately we can't disable the scroll bounce. I've tried it both ways, and it feels really unnatural without it.

I'm open to other ideas, though I realize we are somewhat limited given the behavior of the WebView!

Maybe it's possible to add scroll bounce to a container within the body.

msdewitt · 2024-11-18T21:05:12Z

@raineorshine I think that's possible. However, I'm not as familiar with adding animation and transformations with React as I am with Angular.

I don't think I can complete this ticket since we can't remove the scroll bounce.

Do you want to reopen a new ticket to add a scroll bounce to the native JavaScript?

raineorshine · 2024-11-18T21:15:17Z

@raineorshine I think that's possible. However, I'm not as familiar with adding animation and transformations with React as I am with Angular.

When you say animations and transformations, do you mean the native scroll bounce or a Javascript emulation? Because the Javascript emulations I've seen are never quite performant enough to mimic native functionality.

I don't think I can complete this ticket since we can't remove the scroll bounce.

Are you wanting me to remove it from the milestone?

Do you want to reopen a new ticket to add a scroll bounce to the native JavaScript?

Let's keep the original issue so the full history is there. Changing the way the scroll bounce works is not so much a separate issue as a part of fixing the overscroll background behavior without breaking the existing functionality.

msdewitt · 2024-11-18T21:17:56Z

@raineorshine I mean the javascript emulation. It looks needlessly complicated to implement and might interact weirdly with the UIScrollView of IOS.

The problem with adding the javascript bounce is that it will affect all previous app versions on safari, web, android, and Ios. Where as right now, this is just an IOS issue.

In fact, by disabling the scroll bounce on IOS it is made consistent with all the other versions of the app. There is no scroll bounce similar on web right now and this would be adding a new change across the app.

raineorshine · 2024-11-19T13:40:51Z

@raineorshine I mean the javascript emulation. It looks needlessly complicated to implement and might interact weirdly with the UIScrollView of IOS.

I've looked at all the Javascript emulations that exist for scroll bounce and unfortunately none of them perform well enough to replace the native scroll bounce. We're going to have to stick to native browser scroll bounce. Since this appears to not be possible on the WebView without losing control of the overscroll background, then it should be on an element in the DOM. I think we should explore this latter possibility.

In fact, by disabling the scroll bounce on IOS it is made consistent with all the other versions of the app. There is no scroll bounce similar on web right now and this would be adding a new change across the app.

Not true. We currently have scroll bounce in Chrome, Safari, and Mobile Safari PWA, which is how I use the app personally on a daily basis. em has always had scroll bounce, at least on Apple devices.

msdewitt · 2024-11-26T17:03:42Z

@raineorshine Alright, I managed to fix this issue after a lot of pain. I created a small custom plugin for capacitor and attached it to the project to interact with the native Webview.

msdewitt · 2024-12-28T19:21:20Z

Exactly

msdewitt · 2024-12-29T18:34:14Z

@raineorshine Everything looks good on my end.
Here is a screen recording of me changing the theme, canceling it out, and reloading it.

Try testing again, but don't build or do anything with the plugin version. Just yarn install in the main directory and npm run build and npm run cap:ios and it should be good to go.

Simulator.Screen.Recording.-.iPhone.15.-.2024-12-29.at.12.31.48.mp4

raineorshine · 2024-12-29T19:30:12Z

Thanks for the screen recording. You can see that when you reopen the app it has the wrong background color (black) when the theme is Light.

msdewitt · 2024-12-29T21:59:07Z

@raineorshine I understand, you are talking about the splash screen.

I can't update this initial loading of the splash screen dynamically due to the limits of Capacitor until after the app loads.

When an app using Capacitor loads, it first pulls the capacitor configuration file at the root of the mobile project and won't look at any variables in the app during start-up. We can only update the backgroundColor after the app has loaded through the plugin. It is not possible to update the configuration file of Capacitor before the app starts so we can only make do with a static background color at the start of the app each time.

This is the best we can do and I don't see any way to make further changes to alter the configuration file dynamically on the IOS device after it has been installed.

raineorshine · 2024-12-29T22:03:46Z

No, not the splash screen actually. I'm referring to the overscroll background, the same background indicated in the original issue. You can see it in your video at the very end, after the splash screen goes away. It is set to black, even though the theme is Light.

msdewitt · 2024-12-29T22:34:15Z

@raineorshine Well, maybe I didn't mean splash screen. So this is what I mean.

This file is the configuration file with a preset file for capacitor. It is configured during installation as the set backgroundColor.

During each startup of the app, the app looks at the default backgroundColor set in configuration file and immediately fills in the background color with that value regardless of any changes to the background color from the previous apps changes. If there is no backgroundColor set, it defaults to white.

This happens before the app starts, so I can't even make changes inside the mobile app or pull from app storage to see what the personal settings of the background color are set too.

Now after the app is loaded, only then can we update the background color through the plugin.

Here is the plugin code example:

So at the start of each startup of a capacitor app, the webviewBackground is set to the color in the Capacitor configuration file.

The capacitor configuration file is not designed to be updated dynamically and it should not be stored anywhere accessible on the IOS device itself to update it. I think the configuration values are set as the default during the build of the app in CapacitorBridge file.

So my conclusion is that this change may not possible due to how capacitor is setup.

raineorshine · 2024-12-29T23:02:50Z

Thanks for the additional explanation.

Would it be possible to set the overscroll background color after the app has loaded, and we have had time to read the theme?

Here's what I'm seeing:

black-overscroll-bg-simulator.mp4

msdewitt · 2024-12-29T23:14:22Z

For some reason that mp4 is not working for me.

We do set the background color right after the app loads, which happens right after the webview loads.

In capacitor when the app launches it first loads -> Webview(Native code) -> App (React)
The backgroundColor default is first set in the Webview and then update when the App loads to the users personal settings. So that if you change it to white background and close the app, it will be a white background when you open it again. Its just that the white background is updated at the [App] part of the loading configuration so if the default of the [Webview] is black, then it will first load a black webview and change to a white after.

raineorshine · 2024-12-30T00:03:23Z

How about this? What I'm seeing is that the overscroll background color is black when the theme is Light after the app loads and after the splash screen is gone, as the user is using the app.

black-overscroll-bg-simulator.mov

msdewitt · 2024-12-30T01:56:48Z

Wow, that's really strange. I'm glad you caught that. That shouldn't be happening. When I pulled the latest code and pushed up a fix for it. For some reason, the setting of the theme was not happening and I needed read it back in in the AppComponent.

trevinhofmann

Thank you, @msdewitt!

The overscroll background color appears to be fixed, and the scroll bounce functions as expected 🎉

Both scroll zone parallax and scrollCursorIntoView are also working as expected 👍

ios.ScreenRecording_12-31-2024.00-23-48_1.mp4

1. Initial black overscroll is not resolved until first interaction

The black overscroll is initially displayed when reopening the app in Light mode, and it does not rerender to the correct color until the user interacts with the app. I understand from earlier comments that a limitation with Capacitor's configuration prevents us from resolving the overscroll color until the app/JS loads, but it shouldn't need to wait for user interaction.

Demo:

overscroll.ScreenRecording_12-31-2024.01-07-53_1.mp4

2. Thoughts displayed above toolbar

When scrolling down, thoughts appear above the scrollbar. They should be out of view once they scroll into/above the toolbar.

Current / `bug-issue-1700`:

Expected / `main`:

3. Bulky `node_modules` diff is still in the commit history

Thousands of files were introduced in 973195d and subsequently removed in a5941fd. There may be other commits with this issue, but these are the two I found.

As originally mentioned in this comment, please rebase (squashing 973195d and a5941fd together is one option) to avoid bloating the repository's history with these changes.

ios/App/App/AppDelegate.swift

trevinhofmann · 2024-12-31T06:54:46Z

packages/webview/README.md

+## Install
+
+```bash
+npm install webview-background


I would prefer if we used yarn (possibly other tools/libraries?) consistently with the main project, but I am fine with this if @raineorshine doesn't mind.

Yes, it would be preferable to use the same package manager throughout the project.

Originally suggested here: #2594 (comment)

@msdewitt -- We still have the package-lock.json / npm configuration in this package. Can we please switch to yarn?

src/components/modals/Settings.tsx

trevinhofmann · 2024-12-31T07:16:29Z

packages/webview/dist/esm/index.js

+});
+export * from './definitions';
+export { WebviewBackground };
+//# sourceMappingURL=index.js.map


I would strongly prefer not to include dist files in the repository, and to include building this package as part of building the main project. There are a variety of reasons (repository bloat, merge conflicts more likely, built code more difficult to review for security issues, difficulty verifying that the build matches the source, etc.) for this, but they're relatively unimportant for this particular package due to its small size. I'll defer to @raineorshine who previously indicated that this is probably fine 👍

That was my intuition as well, so I'm glad you said something. I know that @msdewitt did this to simplify the webview package release process, but I would love to discuss the feasibility of building it from source within the root project's build pipeline.

@msdewitt -- Would it be feasible to build from source (as part of the main project's yarn build) rather than committing the dist build to the repo? I imagine we could update the main package.json build command to something like:

"build": "yarn build:webview && yarn build:styles && vite build",

Then add the build:webview script in there as well to build this package from source.

trevinhofmann · 2024-12-31T07:17:52Z

packages/webview/package.json

+  "unpkg": "dist/plugin.js",
+  "files": [
+    "android/src/main/",
+    "android/build.gradle",


Is there an android directory? I don't see one.

Its not needed for this plugin. We can add it later if needed. I deleted it since the fix seemed to only be for IOS.

Makes sense. We can remove the android files from this list in package.json.

Still seeing these which can be removed:

"android/src/main/", "android/build.gradle",

msdewitt · 2025-01-03T05:57:20Z

Alright, I put in the fixes you requested @trevinhofmann . I had to go and change my approach to hide the contentInset on IOS with an ios only CSS adjustments.

I hope to get the approved soon after these changes. I'll rebase after the changes are confirmed in this rework.

trevinhofmann

Thank you, @msdewitt!

These issues appear to be resolved:

Initial black overscroll is not resolved until first interaction

Thoughts displayed above toolbar

The node_modules commits will still need to be rebased/squashed for (3).

4. Toolbar has been shifted down

The toolbar at the top of the page has been shifted down considerably and needs to be positioned back to its original location.

toolbar.mp4

trevinhofmann · 2025-01-03T22:27:00Z

src/e2e/puppeteer-environment.ts

@@ -29,8 +29,6 @@ const PuppeteerEnvironment: Environment = {
      .catch((err: Error) => {
        // using `console.log` here to avoid errors or logs being swallowed by vitest
        // all of `console.error`, `console.warn` and `console.info` don't show up in the terminal
-        // eslint-disable-next-line no-console
-        console.log('Could not connect to browserless.')


Is this change relevant to the PR?

Yes, for some reason lint didn't like it after some changes. Most likely after I merged with main.

We probably want to keep this logging in, as it informs the user if there is any issue with connecting to browserless. The linter does not complain in main, so I suspect that this is a regression.

trevinhofmann · 2025-01-03T22:27:04Z

src/e2e/puppeteer/setup.ts

@@ -50,8 +50,6 @@ const setup = async ({
        break
      case 'info':
      case 'log':
-        // eslint-disable-next-line no-console
-        console[messageType](text)


Is this change relevant to the PR?

Same as above. Lint didn't like it.

trevinhofmann · 2025-01-03T22:32:36Z

packages/webview/dist/esm/web.js

+import { WebPlugin } from '@capacitor/core';
+export class WebviewBackgroundWeb extends WebPlugin {
+    async changeBackgroundColor(options) {
+        console.log('Color changed: ', options.color);


Looks like this log was removed as requested from the source (packages/webview/src/web.ts) but then not rebuilt and included in the build here.

One example of why we should build from source rather than including the dist in the repo :)

trevinhofmann · 2025-01-03T22:41:12Z

src/components/modals/ModalComponent.tsx

    this.animateAndClose!()
    this.props.onClose?.()
+    if (Capacitor.isNativePlatform() && fullReload) {
+      window.location.reload()


I don't believe we should be doing a full page reload here. The only cases where we have those at the moment are for unrecoverable error handling.

Is there a particular reason for doing this? If I understand correctly, it's doing a full reload every time the Settings modal is closed.

We have already had discussions about this in this pr. Its unfortunate that we have to do this, but it is necessary. We can only limit the changes down to the very minimal as possible. (I.E. Only in the settings menu with a flag set as true.)

This is due to a limitation in changing the background color through the WebView. It's bad, but it's the only way to update the overscroll background color from what I understand.

It should reload the page only when the theme setting has changed.

trevinhofmann · 2025-01-03T22:41:59Z

src/components/Toolbar.tsx

@@ -221,9 +222,11 @@ const Toolbar: FC<ToolbarProps> = ({ customize, onSelect, selected }) => {
          }),
        )}
        style={{
+          // height: fontSize + 300,


Unintentional comment? This can be removed.

msdewitt · 2025-01-03T22:53:49Z

@trevinhofmann Please tell me the IOS version you are using to test that toolbar being moved down. In order to cope with your previous review changes about not showing the notes when scrolling down, I had to add a feature which alters the CSS to make the navbar look like it is showing so that it is not covered by the native IOS statusBar.

For some reason, the device you are using is showing that change, but the native IOS StatusBar already has a cover, therefor showing it as too big.

Add changes to prevent ios bounce.

1a1943e

raineorshine requested changes Nov 17, 2024

View reviewed changes

src/App.css Outdated Show resolved Hide resolved

src/App.css Outdated Show resolved Hide resolved

capacitor.config.ts Outdated Show resolved Hide resolved

Matheu DeWitt added 5 commits November 18, 2024 13:12

Add updated css to panda.config file.

fe721c1

Lint

fdec688

Lint

a9a1437

Lint

15ad477

Final lint

89b6c1d

trevinhofmann assigned msdewitt Nov 19, 2024

Matheu DeWitt added 9 commits November 26, 2024 10:35

Save progress

1c95f1f

Ignore file

c0a9da7

Removed custom plugin add.

4611d74

Save progress

69f998a

Remove security issues.

116cc0d

Remove,

932b5fd

Ignore lint

439f1db

Add ignores

12b5ddb

Fix styling

e3978e0

Add file

dcfa432

Readd code to switch theme after startup.

91635d2

raineorshine requested a review from trevinhofmann December 30, 2024 14:13

trevinhofmann assigned trevinhofmann and unassigned msdewitt Dec 31, 2024

trevinhofmann requested changes Dec 31, 2024

View reviewed changes

trevinhofmann assigned msdewitt and unassigned trevinhofmann Dec 31, 2024

Matheu DeWitt and others added 3 commits January 1, 2025 16:39

Remove comment.

a593e29

Add changes for ios top safe area.

ea94d1b

Merge branch 'main' into bug-issue-1700

710b932

msdewitt requested a review from trevinhofmann January 3, 2025 04:13

Matheu DeWitt added 2 commits January 2, 2025 22:16

Add changes to puppeteer console.

8cabd67

Fix lint errors

1c2cd5f

trevinhofmann assigned trevinhofmann and unassigned msdewitt Jan 3, 2025

trevinhofmann requested changes Jan 3, 2025

View reviewed changes

trevinhofmann assigned msdewitt and unassigned trevinhofmann Jan 3, 2025

Fix: [Capacitor] Set overscroll background color from theme #2594

Are you sure you want to change the base?

Fix: [Capacitor] Set overscroll background color from theme #2594

Conversation

msdewitt commented Nov 16, 2024 • edited Loading

msdewitt commented Nov 16, 2024

raineorshine left a comment • edited Loading

Choose a reason for hiding this comment

msdewitt commented Nov 18, 2024

raineorshine commented Nov 18, 2024 • edited Loading

msdewitt commented Nov 18, 2024 • edited Loading

raineorshine commented Nov 18, 2024 • edited Loading

msdewitt commented Nov 18, 2024 • edited Loading

raineorshine commented Nov 18, 2024

msdewitt commented Nov 18, 2024

raineorshine commented Nov 18, 2024

msdewitt commented Nov 18, 2024 • edited Loading

raineorshine commented Nov 19, 2024

msdewitt commented Nov 26, 2024 • edited Loading

msdewitt commented Dec 28, 2024

msdewitt commented Dec 29, 2024

raineorshine commented Dec 29, 2024

msdewitt commented Dec 29, 2024 • edited Loading

raineorshine commented Dec 29, 2024

msdewitt commented Dec 29, 2024 • edited Loading

raineorshine commented Dec 29, 2024

msdewitt commented Dec 29, 2024 • edited Loading

raineorshine commented Dec 30, 2024 • edited Loading

msdewitt commented Dec 30, 2024

trevinhofmann left a comment

Choose a reason for hiding this comment

1. Initial black overscroll is not resolved until first interaction

Demo:

2. Thoughts displayed above toolbar

Current / bug-issue-1700:

Expected / main:

3. Bulky node_modules diff is still in the commit history

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msdewitt commented Jan 3, 2025

trevinhofmann left a comment

Choose a reason for hiding this comment

4. Toolbar has been shifted down

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msdewitt commented Jan 3, 2025

msdewitt commented Nov 16, 2024 •

edited

Loading

raineorshine left a comment •

edited

Loading

raineorshine commented Nov 18, 2024 •

edited

Loading

msdewitt commented Nov 18, 2024 •

edited

Loading

raineorshine commented Nov 18, 2024 •

edited

Loading

msdewitt commented Nov 18, 2024 •

edited

Loading

msdewitt commented Nov 18, 2024 •

edited

Loading

msdewitt commented Nov 26, 2024 •

edited

Loading

msdewitt commented Dec 29, 2024 •

edited

Loading

msdewitt commented Dec 29, 2024 •

edited

Loading

msdewitt commented Dec 29, 2024 •

edited

Loading

raineorshine commented Dec 30, 2024 •

edited

Loading

Current / `bug-issue-1700`:

Expected / `main`:

3. Bulky `node_modules` diff is still in the commit history