Skip to content

Commit

Permalink
fix(header): iOS headers in MD app are not hidden (#29164)
Browse files Browse the repository at this point in the history
Issue number: resolves #28867

---------

<!-- 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. -->

I missed an edge case in
#28277 that caused an
MD headers in an MD app to be hidden due to the presence of an iOS
header (via `mode="ios"`). This was happening because I forgot to scope
the selector in `header.ios.scss` to only iOS headers.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- The headers in the relevant selector are now scoped to the iOS headers
which avoids this bug while preserving the anti-flicker mechanism added
in the linked PR.

## 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.8.1-dev.11710452743.1ca99e5e`

---------

Co-authored-by: ionitron <hi@ionicframework.com>
  • Loading branch information
liamdebeasi and Ionitron committed Mar 15, 2024
1 parent d67fdcc commit fdfecd3
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 2 deletions.
7 changes: 5 additions & 2 deletions core/src/components/header/header.ios.scss
Expand Up @@ -132,8 +132,11 @@
* We use opacity: 0 to avoid a layout shift.
* We target both the attribute and the class in the event that the attribute
* is not reflected on the host in some frameworks.
*
* Both headers should be scoped to iOS mode otherwise an MD app that uses an
* iOS header may cause other MD headers to be unexpectedly hidden.
*/
ion-header:not(.header-collapse-main):has(~ ion-content ion-header[collapse="condense"],
~ ion-content ion-header.header-collapse-condense) {
ion-header.header-ios:not(.header-collapse-main):has(~ ion-content ion-header.header-ios[collapse="condense"],
~ ion-content ion-header.header-ios.header-collapse-condense) {
opacity: 0;
}
88 changes: 88 additions & 0 deletions core/src/components/header/test/basic/header.e2e.ts
Expand Up @@ -85,3 +85,91 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, screenshot, c
});
});
});

/**
* This test only impacts MD applications
*/
configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('header: translucent'), () => {
test('should not hide MD headers when using a descendant iOS header in an MD app', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28867',
});
await page.setContent(
`
<ion-header id="main-header">
<ion-toolbar>
<ion-title>Header</ion-title>
</ion-toolbar>
</ion-header>
<ion-content>
<ion-header collapse="condense">
<ion-toolbar>
<ion-title size="large">Header</ion-title>
</ion-toolbar>
</ion-header>
<ion-header mode="ios">
<ion-toolbar>
<ion-title>Welcome</ion-title>
</ion-toolbar>
</ion-header>
</ion-content>
`,
config
);

const header = page.locator('ion-header#main-header');

/**
* The existence of the iOS header in an MD app should not cause the main MD header
* to be hidden. We do not have toHaveVisible because the behavior that hides
* the header under correct circumstances does it using opacity: 0.
* Playwright considers an element with opacity: 0 to still be visible
* because it has a non-zero bounding box.
*/
await expect(header).toHaveScreenshot(screenshot('header-md-visibility-ios-descendant'));
});
test('should not hide MD headers when using a root iOS header in an MD app', async ({ page }) => {
test.info().annotations.push({
type: 'issue',
description: 'https://github.com/ionic-team/ionic-framework/issues/28867',
});
await page.setContent(
`
<ion-header id="main-header" mode="ios">
<ion-toolbar>
<ion-title>Header</ion-title>
</ion-toolbar>
</ion-header>
<ion-content>
<ion-header collapse="condense">
<ion-toolbar>
<ion-title size="large">Header</ion-title>
</ion-toolbar>
</ion-header>
<ion-header>
<ion-toolbar>
<ion-title>Welcome</ion-title>
</ion-toolbar>
</ion-header>
</ion-content>
`,
config
);

const header = page.locator('ion-header#main-header');

/**
* The existence of the iOS header in an MD app should not cause the main MD header
* to be hidden. We do not have toHaveVisible because the behavior that hides
* the header under correct circumstances does it using opacity: 0.
* Playwright considers an element with opacity: 0 to still be visible
* because it has a non-zero bounding box.
*/
await expect(header).toHaveScreenshot(screenshot('header-md-visibility-ios-main'));
});
});
});
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit fdfecd3

Please sign in to comment.