Skip to content

Conversation

@thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Oct 8, 2025

Issue number: internal


What is the current behavior?

As per accessibility guidelines, there should only be one banner landmark per page. A condensed header creates two banner landmarks since 2 ion-header components are required on the page. ion-header renders with a role="banner" by default (when not in ion-menu).

The visual effect of the condensed header is achieved by rendering two distinct header components. Because both components exist in the code at the same time and both have role="banner", they create a duplicate landmark announcement for screen readers. This leads to a violation with the accessibility guidelines.

What is the new behavior?

  • The role is updated to either none or banner based off the header's active state.
  • Added test.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Preview

@vercel
Copy link

vercel bot commented Oct 8, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
ionic-framework Ready Ready Preview Comment Oct 10, 2025 10:37pm

@github-actions github-actions bot added the package: core @ionic/core package label Oct 8, 2025
@thetaPC thetaPC marked this pull request as ready for review October 8, 2025 19:56
@thetaPC thetaPC requested a review from a team as a code owner October 8, 2025 19:56
@thetaPC thetaPC requested a review from brandyscarney October 8, 2025 19:56
Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to leave this as a comment instead of requesting changes in case this is a non-issue.

* @returns 'none' if inside ion-menu or if condensed in md
* mode, otherwise 'banner'.
*/
export const getRoleType = (isInsideMenu: boolean, isCondensed: boolean, mode: 'ios' | 'md') => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll might have to update the mode to theme in next.

Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice fix!

Copy link
Member

@ShaneK ShaneK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I also especially like the added tests 🚀

@thetaPC thetaPC added this pull request to the merge queue Oct 15, 2025
Merged via the queue into main with commit 12084af Oct 15, 2025
51 checks passed
@thetaPC thetaPC deleted the FW-6767 branch October 15, 2025 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants