fix(overlays): prevent scroll gestures when the overlay is presented #28415

sean-perkins · 2023-10-25T20:16:29Z

Issue number: Resolves #23942

What is the current behavior?

When an overlay is created (inserted in the DOM), but not presented, the scroll gesture is prevented. This behavior comes from the connectedCallback of ion-backdrop, where the gesture is prevented as soon as the backdrop is inserted in the DOM.

This means in situations where a developer creates an overlay, but does not present it immediately, the user cannot scroll. This is not desired.

What is the new behavior?

Scroll blocking behavior tied to the gesture has been removed from ion-backdrop and implemented into the overlays directly.
- When an overlay is presented, scroll blocking is enabled on the body element (the user cannot scroll on the main content).
- When the last presented overlay is dismissed, scroll blocking is disabled on the body element (the user can scroll on the main content).

Does this introduce a breaking change?

Yes
No

ion-backdrop no longer prevents scrolling on the main content when the backdrop is either inserted into the DOM or removed from the DOM. Developers using Ionic overlays do not need to migrate their implementations.

Developers with custom overlays using ion-backdrop internally can either use Ionic's gesture controller to disable scrolling when their overlay is presented/dismissed or can manually add the backdrop-no-scroll Ionic global class to the body element.

Other information

core/src/utils/test/overlays/overlays.e2e.ts

core/src/components/loading/test/loading-id.spec.ts

core/src/utils/test/overlays/overlays.e2e.ts

core/src/components/backdrop/backdrop.tsx

Issue number: N/A ---------   ## What is the current behavior?  Testing Ionicons in Ionic is currently difficult. Developers need to a) create a dev build of Ionicons, b) create a branch in Ionic, c) install the dev build in Ionic core, d) push the branch, and e) create a draft PR to watch the CI process run. ## What is the new behavior?  - Introduces a workflow dispatch option for our CI process. This allows developers to run the CI process for any branch without creating PR. - I also added an optional input so devs can specify the ionicons version if they want to pass a special version. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information

Issue number: Internal --------- ## What is the current behavior? Item sliding has some unused CSS and no tests for safe area padding based on the direction. This CSS is not used: https://github.com/ionic-team/ionic-framework/blob/feda7a0e963048d300eb17d4b9e1056f09088714/core/src/components/item-option/item-option.scss#L20-L30 The rendered markup for a sliding item looks like the following: ```html <ion-item-sliding> <ion-item-options side="start"> <ion-item-option> Archive </ion-item-option> </ion-item-options> <ion-item class="in-list"> <ion-label> Sliding Item </ion-label> </ion-item> </ion-item-sliding> ``` Since `ion-item-options` never gets the `in-list` class added to it, and `ion-item` never contains options, the above CSS is never used. ## What is the new behavior?  - Removed the CSS that is not used, the correct CSS for safe area padding has already been added here: https://github.com/ionic-team/ionic-framework/blob/feda7a0e963048d300eb17d4b9e1056f09088714/core/src/components/item-options/item-options.scss#L57-L67 - Added screenshot tests to verify the safe area padding is applied to the proper side ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information I could add additional tests that make sure there is not padding added when opening the opposite side on each direction but since this problem was happening when changing from `ltr` to `rtl` I did not. [FW-5174]: https://ionic-cloud.atlassian.net/browse/FW-5174?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: ionitron <[email protected]>

@nocollapse

Issue number: resolves #28358 ---------   ## What is the current behavior?  28f2ec9 exposed a (possible) `ng-packagr` bug where the form control components were being re-assigned, which breaks treeshaking. These components were considered side effects and were always being pulled into the bundle. This resulted in a higher than expected bundle size. This issue appears to be caused by using 2 decorators **and** referring to the class in `useExisting` (for providers). Doing just one of these does not reproduce the issue. The compiled output looks something like this: ```typescript let IonToggle = IonToggle_1 = /*@__PURE__*/ class IonToggle extends ValueAccessor { constructor(c, r, z, injector) { super(injector, r); this.z = z; c.detach(); this.el = r.nativeElement; proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']); } writeValue(value) { this.elementRef.nativeElement.checked = this.lastValue = value; setIonicClasses(this.elementRef); } handleIonChange(el) { this.handleValueChange(el, el.checked); } }; /** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component }); /** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [ { provide: NG_VALUE_ACCESSOR, useExisting: IonToggle_1, multi: true, }, ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush }); IonToggle = IonToggle_1 = __decorate([ ProxyCmp({ defineCustomElementFn: defineCustomElement$1i, inputs: TOGGLE_INPUTS, }) ], IonToggle); ``` ## What is the new behavior?  - Removed the `ProxyCmp` usage in favor of manually calling proxyInputs and proxyMethods. - Also saw that select was missing a form control test, so I added one The compiled code now looks something like this: ```typescript class IonToggle extends ValueAccessor { constructor(c, r, z, injector) { super(injector, r); this.z = z; defineCustomElement$1i(); proxyInputs(IonToggle, TOGGLE_INPUTS); c.detach(); this.el = r.nativeElement; proxyOutputs(this, this.el, ['ionChange', 'ionFocus', 'ionBlur']); } writeValue(value) { this.elementRef.nativeElement.checked = this.lastValue = value; setIonicClasses(this.elementRef); } handleIonChange(el) { this.handleValueChange(el, el.checked); } } /** @nocollapse */ IonToggle.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "14.2.12", ngImport: i0, type: IonToggle, deps: [{ token: i0.ChangeDetectorRef }, { token: i0.ElementRef }, { token: i0.NgZone }, { token: i0.Injector }], target: i0.ɵɵFactoryTarget.Component }); /** @nocollapse */ IonToggle.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "14.0.0", version: "14.2.12", type: IonToggle, isStandalone: true, selector: "ion-toggle", inputs: { checked: "checked", color: "color", disabled: "disabled", enableOnOffLabels: "enableOnOffLabels", justify: "justify", labelPlacement: "labelPlacement", legacy: "legacy", mode: "mode", name: "name", value: "value" }, host: { listeners: { "ionChange": "handleIonChange($event.target)" } }, providers: [ { provide: NG_VALUE_ACCESSOR, useExisting: IonToggle, multi: true, }, ], usesInheritance: true, ngImport: i0, template: '<ng-content></ng-content>', isInline: true, changeDetection: i0.ChangeDetectionStrategy.OnPush }); ``` ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  Ryan provided some context on a related Stencil bug where doing reassignments broke treeshaking in Webpack. While the source of this bug is not Stencil, understanding the Stencil bug helped me better understand this issue: ionic-team/stencil#3191 ionic-team/stencil#3248 ionic-team/stencil#4188 (fixes an issue introduced in the above stencil PR) Dev build: `7.5.1-dev.11697480817.10fa2601`

@include

Issue number: N/A ---------   ## What is the current behavior?  While working on a safe area padding mixin, I realized that `rtl()` wasn't being applied for `:host` when using Firefox or Safari. This is happening because the syntax for `:dir()` is wrong. The placement needs to be updated for Firefox and Safari to register it. ```scss :host { @include rtl() { // <- won't work // styles } } // generates :host-context([dir=rtl]) { // styles } :host:dir(rtl) { // <- wrong syntax // styles } ``` ```scss :host(.class) { @include rtl() { // <- won't work // styles } } // generates :host-context([dir=rtl]):host(.class) { // styles } :host-context([dir=rtl]).class { // styles } :host(.class):dir(rtl) { // <- wrong syntax // styles } ``` ## What is the new behavior?  I updated `rtl()` to use `:dir()` as the `addHostSelector` in the `add-root-selector` function. This generates all the correct selectors for Firefox and Safari. However, `:dir()` does not have the structure of `:host-context()` so I had to add a new parameter to `add-root-selector` to determine whether to use `:host-context()` or not. I set the default to `true` since the function originally used `:host-context()`. An extra win is that the updated function will be ready for when `:host-context()` can be removed from the codebase. ```diff :host { @include rtl() { // <- works // styles } } // generates :host-context([dir=rtl]) { // styles } - :host:dir(rtl) { + :host(:dir(rtl)) { // styles } ``` ```diff :host(.class) { @include rtl() { // <- works // styles } } // generates :host-context([dir=rtl]):host(.class) { padding-right: 40px; } :host-context([dir=rtl]).class { // styles } - :host(.class):dir(rtl) { + :host(.class:dir(rtl)) { // styles } ``` ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  N/A --------- Co-authored-by: ionitron <[email protected]>

Issue number: N/A ---------   ## What is the current behavior?  Prettier is configured to ignore the `proxies.ts` file in the `src` directory only. This means it is adjusting whitespace/commas/etc on the `proxies.ts` file in the `standalone` directory which we do not want because we will always get diffs whenever `npm run build` is run in `core`. ## What is the new behavior?  - Updated prettier config to ignore all `proxies.ts` files in the `angular` package - Re-generated the proxies file ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information

Issue number: resolves #18683, resolves #15538, resolves #22341 ---------   ## What is the current behavior?  Menus in a split pane are hidden when a second split pane is mounted/made visible. This is because the `onSplitPaneChanged` callback does not take into account whether the it is a child of the split pane that emitted `ionSplitPaneVisible`. When split pane 2 is shown, that causes the menu is split pane 1 to hide. When split pane 1 is shown, the menu inside of it _is_ shown. However, since split pane 2 is then hidden that component also emits `ionSplitPaneVisible`, causing the menu inside of split pane 1 to hide. ## What is the new behavior?  - Menus are only hidden when its parent split pane changes visibility ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  Dev build: `7.5.1-dev.11697568647.1ac87d08` --------- Co-authored-by: Amanda Johnston <[email protected]>

Issue number: N/A ---------   ## What is the current behavior?  This project has several unused Sass variables still in the code base. The team would like to remove these. ## What is the new behavior?  - Removed unused Sass variables ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  The original scope of this ticket was for checkbox only, but many other components had unused sass variables, so I decided to tackle everything all at once. Since these variables are not used anywhere: 1. The build should pass 2. There should be no screenshot diffs

… direction (#28377) Issue number: Internal ---------   ## What is the current behavior?  When calculating the fab's horizontal position, the safe area is taken into account. However, which safe area side is applied changes depending on whether the document's direction is LTR or RTL. This is incorrect as the left safe area padding will always be on the left side regardless of direction, and vice versa. ## What is the new behavior?  The left safe area is always applied to the fab's `left` position, and vice versa. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  --------- Co-authored-by: ionitron <[email protected]>

Issue number: Internal ---------   ## What is the current behavior?  As part of FW-2832, the team would like to swap out usages of the any type for stronger types. ## What is the new behavior?  c529bc2 - `scrollToTop` doesn't return anything, so I added the `void` return type a96971a - `animation.effect` is a type of [AnimationEffect](https://developer.mozilla.org/en-US/docs/Web/API/Animation/effect). One of the more common types of effects is a `KeyframeEffect`. However, TypeScript doesn't know which specific type of AnimationEffect we are using, so I cast `animation.effect` as KeyframeEffect where appropriate. - I also added `!` to places where we know the effect and other properties are always defined (since they run after the web animation has been constructed) - Added stronger types to the internal to/from/fromTo functions (the public facing type improvements are in #28334) fdaf550 - `getRootNode` can return multiple types of objects, so I cast it to the specific types that we work with in `isFocused`. 46a6efa - Added the "Animation" type and resolved related errors once we had stronger types a7cb9a5 - Made heavier use of the `T` generic - Once we know `node` is an Element (`nodeType === 1`) we manually cast the element as `T` 6a9d1f0 - The focus visible utility is an internal utility, but it was lacking an interface, so I added one. 90b64c2 - Removed unneeded HTMLElement casting - Added `!` since we can assume the selected elements are defined with the refresher - Added documentation as to why casting `referencEl.style` as `any` is something we need to keep. 3a084ca - Avoided the Event naming collision by using globalThis ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  Note: This PR contains only type changes. Changes the required updates to the implementation of Ionic are pulled out into separate PRs and target a minor release branch to minimize risk. --------- Co-authored-by: Amanda Johnston <[email protected]>

…le (#28362) Issue number: resolves #28283 ---------   ## What is the current behavior?  `getInputElement()` is used to access the native input. If the component has yet to render, then the function will return `undefined`. This happens mostly when using `ref` on React. ```tsx <IonInput ref={async input => { const nativeInput = await input.getInputElement(); // nativeInput is undefined }} /> ``` ## What is the new behavior?  - `getInputElement()` will wait to return once the component is ready. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  Dev build: `7.5.1-dev.11697488622.175c9183`

Issue number: internal ---------   ## What is the current behavior?  Legacy radio has tests that use `Tab` key presses. These tend to flake at unknown moments and were skipped until a fix can be implemented. ## What is the new behavior?  Unable to replicate the flakiness locally or on GitHub. However, it only fails on Safari so the tests were re-enabled except for Safari. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  This only happens on legacy. The legacy form controls will also be removed in the future. Due to this, these tests will be removed anyways at that time.

…8369) Issue number: resolves #18487 ---------   ## What is the current behavior?  Web-based users do not get a scrollbar when: - alert has a long list of inputs (this also happens on `ion-select` with the alert interface) - `ion-select` uses the action-sheet interface and has a long list of options This makes it difficult for users to navigate through the options by forcing them to use their keyboards. Some users may also not be used to using their keyboards for navigation. Additionally, this can lead to potential confusion that there are no other options. ## What is the new behavior?  Web-based users get a scrollbar when: - alert has a long list of inputs (this also happens on `ion-select` with the alert interface) - `ion-select` uses the action-sheet interface and has a long list of options ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  The issue was filed for the alert interface but it's also happening on the action-sheet interface. Dev build: 7.5.1-dev.11697570585.1774584d

This commit removes the blocking behavior as part of the backdrop. Developers with custom implementations using ion-backdrop that want scroll blocking will either need to use Ionic's gesture controller API or manually add the backdrop-no-scroll class to the body when their overlay is presented and remove it when dismissed.

core/src/components/action-sheet/action-sheet.tsx

core/src/utils/overlays.ts

liamdebeasi

I'm good with the change as-is, just one question.

github-actions bot added the package: core @ionic/core package label Oct 25, 2023

sean-perkins marked this pull request as ready for review October 26, 2023 14:11

sean-perkins requested a review from averyjohnston as a code owner October 26, 2023 14:11

sean-perkins commented Oct 26, 2023

View reviewed changes

core/src/utils/test/overlays/overlays.e2e.ts Outdated Show resolved Hide resolved

sean-perkins commented Oct 26, 2023

View reviewed changes

core/src/components/loading/test/loading-id.spec.ts Outdated Show resolved Hide resolved

sean-perkins requested a review from liamdebeasi October 26, 2023 14:14

liamdebeasi reviewed Oct 26, 2023

View reviewed changes

core/src/utils/test/overlays/overlays.e2e.ts Outdated Show resolved Hide resolved

averyjohnston reviewed Oct 26, 2023

View reviewed changes

core/src/components/backdrop/backdrop.tsx Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

sean-perkins changed the base branch from main to feature-8.0 October 27, 2023 16:54

sean-perkins requested review from brandyscarney and thetaPC as code owners October 27, 2023 16:54

dependabot bot and others added 18 commits October 27, 2023 12:55

v7.5.1

5946783

chore(): update package lock files

0d3d834

chore: clean up changelog

d9b8f36

sean-perkins added 7 commits October 27, 2023 12:59

chore: build

ed24f9d

test: delay before interaction

bfd8601

fix: actually disable scroll

c4317bf

test: validate scroll blocking behavior in unit test

521b82a

test: split test cases

12e2178

fix: only enable scroll when last overlay is dismissed

55bb326

sean-perkins force-pushed the sp/FW-196 branch from 99bffe2 to a3a1d35 Compare October 27, 2023 17:02

github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Oct 27, 2023

Merge remote-tracking branch 'origin/feature-8.0' into sp/FW-196

532a9b2

github-actions bot removed package: angular @ionic/angular package package: vue @ionic/vue package package: react @ionic/react package labels Oct 30, 2023

sean-perkins added 2 commits October 30, 2023 12:56

chore: remove backdrop import from spec tests

c6bc0e7

test: stable testing method for class name

8e88269

sean-perkins commented Oct 30, 2023

View reviewed changes

core/src/components/action-sheet/action-sheet.tsx Outdated Show resolved Hide resolved

sean-perkins added 4 commits October 30, 2023 17:53

chore: split test file to workaround stencil bug

fd3b08a

chore: clean-up

7624fda

chore: simplify approach

b6af335

chore: revert unnecessary diffs

dd2944d

sean-perkins requested review from liamdebeasi and averyjohnston October 31, 2023 17:17

liamdebeasi reviewed Oct 31, 2023

View reviewed changes

core/src/utils/overlays.ts Outdated Show resolved Hide resolved

liamdebeasi approved these changes Oct 31, 2023

View reviewed changes

chore: remove class before dismiss transition

45dadca

sean-perkins merged commit 7ba939f into feature-8.0 Nov 1, 2023
43 checks passed

sean-perkins deleted the sp/FW-196 branch November 1, 2023 18:58

liamdebeasi mentioned this pull request Dec 14, 2023

bug: overlays block scroll as soon as they are created, even if they do not get presented #23942

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(overlays): prevent scroll gestures when the overlay is presented #28415

fix(overlays): prevent scroll gestures when the overlay is presented #28415

sean-perkins commented Oct 25, 2023 •

edited

Loading

This comment was marked as resolved.

liamdebeasi left a comment

fix(overlays): prevent scroll gestures when the overlay is presented #28415

fix(overlays): prevent scroll gestures when the overlay is presented #28415

Conversation

sean-perkins commented Oct 25, 2023 • edited Loading

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

This comment was marked as resolved.

liamdebeasi left a comment

Choose a reason for hiding this comment

sean-perkins commented Oct 25, 2023 •

edited

Loading