fix(core): allow fullscreen scroll content to flow outside container for translucent tab bar - #28246
Merged
Conversation
2 tasks
liamdebeasi
marked this pull request as ready for review
September 27, 2023 19:05
liamdebeasi
requested review from
brandyscarney and
sean-perkins
as code owners
September 27, 2023 19:05
sean-perkins
approved these changes
Oct 4, 2023
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Oct 4, 2023
…28245) Issue number: resolves #21130 --------- <!-- 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. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> IonContent sets `--offset-top` and `--offset-bottom` variables to allow the content to scroll under headers, footers, and tab bars. This is essential to creating the translucency effect on these components. IonContent does this by computing its offsetHeight and offsetTop coordinates which take into account the dimensions of headers, footers, and tab bars. Occasionally, this code will run before the IonTabBar has been hydrated which means that the offset will be wrong because the IonTabBar will have a dimension of 0x0 prior to hydration. This impacts Ionic Angular devs who are using the lazy loaded build of Ionic. React and Vue devs are not impacted because they are using the dist-custom-elements build of Ionic which does not have hydration. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - IonContent will re-run the offset computation code whenever the `ionTabBarLoaded` event is emitted. This event is emitted at most once per IonTabBar instance. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change, please describe the impact and migration path for existing applications below. --> ## Other information <!-- 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.4.2-dev.11695831341.191bdf12` Note: I did not write a test since this is fixing a race condition. I wasn't able to find a non-flaky way of testing this. You can test this in an Ionic Angular Tabs starter application with the dev build. The `--offset-bottom` variable on `ion-content` should be large enough such that the content will scroll under the tab bar. The translucency effect won't work just yet, but that is being fixed in #28246.
liamdebeasi
enabled auto-merge
October 4, 2023 18:54
2 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Dec 21, 2023
…arger screens (#28745) Issue number: resolves #28737 --------- <!-- 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. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> In #28246 we removed the overflow on Nav and Router Outlet to allow content to flow outside of these containers. This allows the translucent tab effect to work (otherwise content would be clipped and there would be no translucency). However, this had the unintended side effect of causing page transitions to flow outside of these components. This is most noticeable on larger displays when using SplitPane because the page can cover the menu mid-transition. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> - Overflow is no longer allowed on the main content. I originally set the overflow on the router outlet/nav itself, but that caused the translucent tab bar behavior to regress. Since this issue only happens with split pane, I decided to apply the fix there. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- 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.6.3-dev.11703103144.148eb1f6`⚠️ Please test in a physical app too ### Before/After Demo | `main` | branch | | - | - | | <video src="https://github.com/ionic-team/ionic-framework/assets/2721089/2f3f0d74-9a7e-4ebe-b58a-6e1a6ea3636e"></video> | <video src="https://github.com/ionic-team/ionic-framework/assets/2721089/cdbf8fb5-e217-48cf-8c1e-4bdecee4de4c"></video> | ### Screenshot Diffs The `menu-custom` screenshot diffs ([Example](https://github.com/ionic-team/ionic-framework/pull/28745/files)) are correct. By adding `overflow: hidden`, the box shadow from the "Content" header no longer flows outside of the container. The menu [border on MD has an opacity of 0.18](https://github.com/ionic-team/ionic-framework/blob/e5226016a0f0b066a7bd7fc9997f905d3b87fbc4/core/src/components/menu/menu.md.vars.scss#L7), so the border color was mixing with the color of the box shadow from the header. Since the shadow no longer flows outside of the container, the border color does not mix with the box shadow color. ### Does this break translucent tabs when the split pane is inside of a tab? No. The [split pane has `contain: strict` set](https://github.com/ionic-team/ionic-framework/blob/e5226016a0f0b066a7bd7fc9997f905d3b87fbc4/core/src/components/split-pane/split-pane.scss#L24) which prevents content from flowing under the tab bar, so the translucent tab bar never worked with this layout. --------- Co-authored-by: ionitron <hi@ionicframework.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 #17676
What is the current behavior?
The IonNav, IonRouterOutlet, and .ion-page elements have
overflow: hiddenwhich prevent content from spilling out of it. This was likely done to prevent these elements from having overflow scroll (since the inner IonContent should be scrollable). However, this breaks the translucency effect on IonTabBar because the content in IonContent can not scroll under the IonTabBar.In Ionic v3 components such as IonTabs and IonNav did have
overflow: hidden: https://github.com/ionic-team/ionic-v3/blob/cf35d5eb7f38d088e7c2588e42738228c39e9964/src/components/app/app.scss#L241-L246However, components like IonNav were not used inside of tabs at the time, so the reported bug was not a problem then.
What is the new behavior?
overflow: hiddenfrom IonNav, IonRouterOutlet, and .ion-page. This change seems safe to make because theposition: absoluteand top/right/bottom/left values should ensure that these elements take up the available screen space and avoid having overflow scrolling.Does this introduce a breaking change?
Other information
Dev build:
7.4.2-dev.11695832397.13fa6703Note: Fixing this reveals #21130 which is why this fix is dependent on the linked issue getting fixed first.