fix(footer, tab-bar): wait for resize before re-showing #27417
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
liamdebeasi
changed the title
liamdebeasi
commented
May 8, 2023
| import { raf } from '@utils/helpers'; | ||
| import { win } from '@utils/window'; | ||
|
|
||
| import { KeyboardResize, Keyboard } from '../native/keyboard'; |
There was a problem hiding this comment.
This gave me a strange build error when I tried to do @utils/native/keyboard:
[ ERROR ] Rollup: Unresolved Import
Could not resolve '../native/keyboa' from ./src/utils/keyboard/keyboard-controller.ts
There was a problem hiding this comment.
Was this resolved? I wasn't able to reproduce any build errors.
There was a problem hiding this comment.
I'm still getting the issue locally when doing npm run start:
[ ERROR ] Rollup: Unresolved Import
Could not resolve '../native/keyboa' from
./src/utils/keyboard/keyboard-controller.ts
There was a problem hiding this comment.
I'm also not able to reproduce any issues when running npm run start. Does it happen as soon as the command is entered or does it show when on a specific page?
There was a problem hiding this comment.
I'm getting the error as soon as I run npm start, or at least a bit later once the build has finished. I did also try rm ./dist and npm install in case I had some build cache or oudated Stencil issues, but the error's still occuring. Odd 🤔
There was a problem hiding this comment.
I can ask the Stencil team about this during our sync today, but I don't think it's worth holding up this PR over.
There was a problem hiding this comment.
Super weird, I'll be curious to know the outcome.
There was a problem hiding this comment.
Spoke with Ryan, and he thinks the compiler may be expecting to see a ts extension since only 2 characters are being cut off (i.e. the length of the file extension). Tanner is working on a similar bug, and Ryan had me try dev build with Tanner's proposed fix. However, the proposed fix did not change the behavior here.
There was a problem hiding this comment.
For posterity, the fix for this is tied to ionic-team/stencil#4481
There was a problem hiding this comment.
Fix landed/released in Stencil v3.4.1 - https://github.com/ionic-team/stencil/releases/tag/v3.4.1
thetaPC
requested changes
May 16, 2023
averyjohnston
approved these changes
May 16, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 tasks
brandyscarney
pushed a commit
that referenced
this pull request
May 22, 2023
Issue number: resolves #25990 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> <!-- Please describe the current behavior that you are modifying. --> The tab bar and footer are being shown too soon after the keyboard begins to hide. This is happening because the webview resizes _after_ the keyboard begins to dismiss. As a result, it is possible for the tab bar and footer to briefly appear on the top of the keyboard in environments where the webview resizes. <!-- Please describe the behavior or changes that are being added by this PR. --> - The tab bar and footer wait until after the webview has resized before showing again | before | after | | - | - | | <video src="https://user-images.githubusercontent.com/2721089/236905066-42ac17a5-a5bf-458b-9c62-005fcce05e20.MP4"></video> | <video src="https://user-images.githubusercontent.com/2721089/236905185-d2f539d1-6d93-4385-b1cb-24dd7aa06393.MP4"></video> | This code works by adding an optional parameter to the keyboard controller callback called `waitForResize`. When defined, code within Ionic can wait for the webview to resize as a result of the keyboard opening or closing. Tab bar and footer wait for this `waitForResize` promise to resolve before re-showing the relevant elements. This `waitForResize` parameter is only only defined when all of the following are two: **1. The webview resize mode is known and is _not_ "None".** If the webview resize mode is unknown then either the Keyboard plugin is not installed (in which case the tab bar/footer are never hidden in the first place) or the app is being deployed in a browser/PWA environment (in which case the web content typically does not resize). If the webview resize mode is "None" then that means the keyboard plugin is installed, but the webview is configured to never resize when the keyboard opens/closes. As a result, there is no need to wait for the webview to resize. **2. The webview has previously resized.** If the keyboard is closed _before_ the opening keyboard animation completes then it is possible for the webview to never resize. In this case, the webview is at full height and the tab bar/footer can immediately be re-shown. ------ Under the hood, we use a [ResizeObserver](https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver) to listen for when the web content resizes. Which element we listen on depends on the resize mode set in the developer's Capacitor app. We determine this in the `getResizeContainer` function. From there, we wait for the ResizeObserver callback, then wait 1 more frame so the promise resolves _after_ the resize has finished. - [ ] Yes - [x] No <!-- If this introduces a breaking change, please describe the impact and migration path for existing applications below. --> <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Dev build: `7.0.6-dev.11683905366.13943af0`
alicewriteswrongs
added a commit
to ionic-team/stencil
that referenced
this pull request
Jun 13, 2023
This fixes an issue (STENCIL-840) which is mentioned here: ionic-team/ionic-framework#27417 (comment) The problem is that for _certain_ aliased paths more of the resolve filepath is getting cut off than intended. In the example referenced in the discussion above, for instance, the resolved path should look something like `../native/keyboard` but instead it looks like `../native/keyboa`. In `rewriteAliasedSourceFileImportPaths` we rewrite aliased paths by basically: 1. resolving the full path to the aliased module (so e.g. `@utils` is resolved to `src/utils/index.ts`) 2. calculating the relative path from the importing module (where there is an import like `import { foo } from '@utils';`). This might look something like `../../utils/index.ts`. 3. re-writing the resolved relative path to remove the file extension, giving us something like `../../utils/index`. The problem arose in the third step. In order to get complete coverage for all the possible file extensions we could run into we programmatically construct a regular expression based on `ts.Extension`. On `main` at present this looks like this: ```ts const extensionRegex = new RegExp( Object.values(ts.Extension) .map((extension) => `${extension}$`) .join('|') ); ``` The problem is that `Object.values(ts.Extension)` looks like this: ```ts [ '.ts', '.tsx', '.d.ts', '.js', '.jsx', '.json', '.tsbuildinfo', '.mjs', '.mts', '.d.mts', '.cjs', '.cts', '.d.cts' ] ``` Thus our regular expression will end up having patterns in it like ```ts /.d.ts$/ ``` This will match the substring `rd.ts` at the end of a string like `../native/keyboard.ts`, because `.` is the wildcard and will match any character. This is not what we want! The solution is to replace `"."` in the extensions with `"\\."` to match only the string `"."`, which was the original intention.
alicewriteswrongs
added a commit
to ionic-team/stencil
that referenced
this pull request
Jun 13, 2023
This fixes an issue (STENCIL-840) which is mentioned here: ionic-team/ionic-framework#27417 (comment) The problem is that for _certain_ aliased paths more of the resolve filepath is getting cut off than intended. In the example referenced in the discussion above, for instance, the resolved path should look something like `../native/keyboard` but instead it looks like `../native/keyboa`. In `rewriteAliasedSourceFileImportPaths` we rewrite aliased paths by basically: 1. resolving the full path to the aliased module (so e.g. `@utils` is resolved to `src/utils/index.ts`) 2. calculating the relative path from the importing module (where there is an import like `import { foo } from '@utils';`). This might look something like `../../utils/index.ts`. 3. re-writing the resolved relative path to remove the file extension, giving us something like `../../utils/index`. The problem arose in the third step. In order to get complete coverage for all the possible file extensions we could run into we programmatically construct a regular expression based on `ts.Extension`. On `main` at present this looks like this: ```ts const extensionRegex = new RegExp( Object.values(ts.Extension) .map((extension) => `${extension}$`) .join('|') ); ``` The problem is that `Object.values(ts.Extension)` looks like this: ```ts [ '.ts', '.tsx', '.d.ts', '.js', '.jsx', '.json', '.tsbuildinfo', '.mjs', '.mts', '.d.mts', '.cjs', '.cts', '.d.cts' ] ``` Thus our regular expression will end up having patterns in it like ```ts /.d.ts$/ ``` This will match the substring `rd.ts` at the end of a string like `../native/keyboard.ts`, because `.` is the wildcard and will match any character. This is not what we want! The solution is to replace `"."` in the extensions with `"\\."` to match only the string `"."`, which was the original intention.
alicewriteswrongs
added a commit
to ionic-team/stencil
that referenced
this pull request
Jun 22, 2023
This fixes an issue (STENCIL-840) which is mentioned here: ionic-team/ionic-framework#27417 (comment) The problem is that for _certain_ aliased paths more of the resolve filepath is getting cut off than intended. In the example referenced in the discussion above, for instance, the resolved path should look something like `../native/keyboard` but instead it looks like `../native/keyboa`. In `rewriteAliasedSourceFileImportPaths` we rewrite aliased paths by basically: 1. resolving the full path to the aliased module (so e.g. `@utils` is resolved to `src/utils/index.ts`) 2. calculating the relative path from the importing module (where there is an import like `import { foo } from '@utils';`). This might look something like `../../utils/index.ts`. 3. re-writing the resolved relative path to remove the file extension, giving us something like `../../utils/index`. The problem arose in the third step. In order to get complete coverage for all the possible file extensions we could run into we programmatically construct a regular expression based on `ts.Extension`. On `main` at present this looks like this: ```ts const extensionRegex = new RegExp( Object.values(ts.Extension) .map((extension) => `${extension}$`) .join('|') ); ``` The problem is that `Object.values(ts.Extension)` looks like this: ```ts [ '.ts', '.tsx', '.d.ts', '.js', '.jsx', '.json', '.tsbuildinfo', '.mjs', '.mts', '.d.mts', '.cjs', '.cts', '.d.cts' ] ``` Thus our regular expression will end up having patterns in it like ```ts /.d.ts$/ ``` This will match the substring `rd.ts` at the end of a string like `../native/keyboard.ts`, because `.` is the wildcard and will match any character. This is not what we want! The solution is to replace `"."` in the extensions with `"\\."` to match only the string `"."`, which was the original intention.
alicewriteswrongs
added a commit
to ionic-team/stencil
that referenced
this pull request
Jun 22, 2023
This fixes an issue (STENCIL-840) which is mentioned here: ionic-team/ionic-framework#27417 (comment) The problem is that for _certain_ aliased paths more of the resolve filepath is getting cut off than intended. In the example referenced in the discussion above, for instance, the resolved path should look something like `../native/keyboard` but instead it looks like `../native/keyboa`. In `rewriteAliasedSourceFileImportPaths` we rewrite aliased paths by basically: 1. resolving the full path to the aliased module (so e.g. `@utils` is resolved to `src/utils/index.ts`) 2. calculating the relative path from the importing module (where there is an import like `import { foo } from '@utils';`). This might look something like `../../utils/index.ts`. 3. re-writing the resolved relative path to remove the file extension, giving us something like `../../utils/index`. The problem arose in the third step. In order to get complete coverage for all the possible file extensions we could run into we programmatically construct a regular expression based on `ts.Extension`. On `main` at present this looks like this: ```ts const extensionRegex = new RegExp( Object.values(ts.Extension) .map((extension) => `${extension}$`) .join('|') ); ``` The problem is that `Object.values(ts.Extension)` looks like this: ```ts [ '.ts', '.tsx', '.d.ts', '.js', '.jsx', '.json', '.tsbuildinfo', '.mjs', '.mts', '.d.mts', '.cjs', '.cts', '.d.cts' ] ``` Thus our regular expression will end up having patterns in it like ```ts /.d.ts$/ ``` This will match the substring `rd.ts` at the end of a string like `../native/keyboard.ts`, because `.` is the wildcard and will match any character. This is not what we want! The solution is to replace `"."` in the extensions with `"\\."` to match only the string `"."`, which was the original intention.
github-merge-queue bot
pushed a commit
to ionic-team/stencil
that referenced
this pull request
Jun 22, 2023
This fixes an issue (STENCIL-840) which is mentioned here: ionic-team/ionic-framework#27417 (comment) The problem is that for _certain_ aliased paths more of the resolve filepath is getting cut off than intended. In the example referenced in the discussion above, for instance, the resolved path should look something like `../native/keyboard` but instead it looks like `../native/keyboa`. In `rewriteAliasedSourceFileImportPaths` we rewrite aliased paths by basically: 1. resolving the full path to the aliased module (so e.g. `@utils` is resolved to `src/utils/index.ts`) 2. calculating the relative path from the importing module (where there is an import like `import { foo } from '@utils';`). This might look something like `../../utils/index.ts`. 3. re-writing the resolved relative path to remove the file extension, giving us something like `../../utils/index`. The problem arose in the third step. In order to get complete coverage for all the possible file extensions we could run into we programmatically construct a regular expression based on `ts.Extension`. On `main` at present this looks like this: ```ts const extensionRegex = new RegExp( Object.values(ts.Extension) .map((extension) => `${extension}$`) .join('|') ); ``` The problem is that `Object.values(ts.Extension)` looks like this: ```ts [ '.ts', '.tsx', '.d.ts', '.js', '.jsx', '.json', '.tsbuildinfo', '.mjs', '.mts', '.d.mts', '.cjs', '.cts', '.d.cts' ] ``` Thus our regular expression will end up having patterns in it like ```ts /.d.ts$/ ``` This will match the substring `rd.ts` at the end of a string like `../native/keyboard.ts`, because `.` is the wildcard and will match any character. This is not what we want! The solution is to replace `"."` in the extensions with `"\\."` to match only the string `"."`, which was the original intention.
2 tasks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #25990
What is the current behavior?
The tab bar and footer are being shown too soon after the keyboard begins to hide. This is happening because the webview resizes after the keyboard begins to dismiss. As a result, it is possible for the tab bar and footer to briefly appear on the top of the keyboard in environments where the webview resizes.
What is the new behavior?
RPReplay_Final1683569175.MP4
RPReplay_Final1683570992.MP4
This code works by adding an optional parameter to the keyboard controller callback called
waitForResize. When defined, code within Ionic can wait for the webview to resize as a result of the keyboard opening or closing. Tab bar and footer wait for thiswaitForResizepromise to resolve before re-showing the relevant elements.This
waitForResizeparameter is only only defined when all of the following are two:1. The webview resize mode is known and is not "None".
If the webview resize mode is unknown then either the Keyboard plugin is not installed (in which case the tab bar/footer are never hidden in the first place) or the app is being deployed in a browser/PWA environment (in which case the web content typically does not resize). If the webview resize mode is "None" then that means the keyboard plugin is installed, but the webview is configured to never resize when the keyboard opens/closes. As a result, there is no need to wait for the webview to resize.
2. The webview has previously resized.
If the keyboard is closed before the opening keyboard animation completes then it is possible for the webview to never resize. In this case, the webview is at full height and the tab bar/footer can immediately be re-shown.
Under the hood, we use a ResizeObserver to listen for when the web content resizes. Which element we listen on depends on the resize mode set in the developer's Capacitor app. We determine this in the
getResizeContainerfunction.From there, we wait for the ResizeObserver callback, then wait 1 more frame so the promise resolves after the resize has finished.
Does this introduce a breaking change?
Other information
Dev build:
7.0.6-dev.11683905366.13943af0