fix(tab-button): match height to tab-bar in ionic-md theme#31209
fix(tab-button): match height to tab-bar in ionic-md theme#31209OS-jacobbell wants to merge 16 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
thetaPC
left a comment
There was a problem hiding this comment.
Hey! Great catch on tracking it down to line-height. That's exactly right, and the diagnostic was spot on. Wanted to share why it works and propose a small adjustment.
Why your fix works
The typography mixin applied to :host sets line-height: 20px (from body-action-xs). Because line-height inherits, that 20px cascades into the slotted <ion-icon>. The icon is 24px tall, so it's bigger than the inherited line.
Firefox handles "inline-block taller than its line-height" by doing baseline math that lands on sub-pixel values, and those values can shift by tiny fractions between paint passes. Same layout (the boundingBox() was identical on flaky and passing runs), but anti-aliasing on the icon edges and focus ring renders slightly differently each time. Enough to fail the snapshot.
By dropping line-height from the host, the icon falls back to whatever's inherited from the ancestor chain (currently body), and the mismatch goes away.
Why it only surfaces in tab-button
Three conditions have to line up, and tab-button is the only place all three hold today:
The host applies a typography token whose line-height (20px) is smaller than the pinned icon size (24px). Most components inherit body-md-regular (24px), which matches.
There's a thin visual detail nearby (the 2px focus outline on the rounded corners) where even a fractional pixel shift becomes visible in a screenshot diff.
The snapshot test captures that exact state (the focused tab is in the test).
Other components with ::slotted(ion-icon) have condition 1 latent, but they're missing 2 or 3, so the bug is silent there.
Why it's actually a coincidence
body in typography.ionic.scss:17 applies body-md-regular, whose line-height resolves to 24px. That happens to match the icon's size exactly. 24 == 24 means no mismatch, no baseline math, no flake.
The moment someone changes the body token (or any wrapping component sets a smaller line-height), the icon inherits something less than 24px again and the flake comes back silently. We'd have no signal it regressed until snapshots start failing on Firefox.
Suggested adjustment
Revert the host-side change and instead pin line-height directly on the slotted icon, where the size is already pinned:
::slotted(ion-icon) {
width: globals.$ion-scale-600;
height: globals.$ion-scale-600;
+ line-height: normal;
}Now the icon's line-height is locally controlled and immune to whatever any ancestor decides. The fix becomes invariant rather than coincidence dependent.
Worth noting this same pattern applies anywhere we pin an icon size via ::slotted(ion-icon). There are roughly 20 components doing it. Probably worth targeting ion-icon at a global level (e.g. in typography.ionic.scss) so every wrapping component is protected at once and future migrations don't have to remember this. The tab-button fix here is still the immediate one, with the global change as a follow up.
One meta thing
While you're updating the PR, could you include the root cause in the description? Something like the first two sections above. What the actual mechanism is (inherited line-height plus inline-block baseline math in Firefox), not just what changed. It helps reviewers and future readers tell whether a change addresses the underlying bug or only makes the symptom go away. In this case the line-height observation was the real insight. Capturing it in the PR means the next person hitting flakiness on a similar test has a starting point instead of re-deriving it from scratch.
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
There was a problem hiding this comment.
This snapshot is inaccurate when it comes to the badges. Notice that the bottom bar has the badges shifted. Review my comment on the styles file to address this.
| /* | ||
| * When the icon has a top margin (icon-top layout with both icon and | ||
| * label), shift the badges down by the same amount so they stay anchored | ||
| * to the icon visually rather than to the tab-button's outer box. | ||
| */ | ||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge) { | ||
| top: calc(globals.$ion-scale-100 * -1); | ||
| } | ||
|
|
||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge:empty) { | ||
| top: 0; | ||
| } | ||
|
|
||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top) { | ||
| top: 2px; | ||
| } | ||
|
|
||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top:empty) { | ||
| top: 0; | ||
| } | ||
|
|
||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-bottom) { | ||
| top: calc(50% - globals.$ion-space-100); | ||
| } | ||
|
|
||
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-bottom:empty) { | ||
| top: 50%; | ||
| } |
There was a problem hiding this comment.
Quick context on the original badge override suggestions: those were a starting point, not tested code. The expectation is that the implementer tests them locally and adjusts if the math doesn't pan out visually. That's the part I should have flagged more clearly up front.
After looking at the snapshots in this PR, I tested locally and the math I gave you was off. The icon doesn't actually shift by 4px when the margin is added, it shifts by 2px (the margin replaces the 2px of free space that flex centering was distributing above the icon). So most of the badge overrides I suggested were over-correcting and producing the wrong position.
You can drop the other four overrides. Sorry for the back and forth on this one.
| /* | |
| * When the icon has a top margin (icon-top layout with both icon and | |
| * label), shift the badges down by the same amount so they stay anchored | |
| * to the icon visually rather than to the tab-button's outer box. | |
| */ | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge) { | |
| top: calc(globals.$ion-scale-100 * -1); | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge:empty) { | |
| top: 0; | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top) { | |
| top: 2px; | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top:empty) { | |
| top: 0; | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-bottom) { | |
| top: calc(50% - globals.$ion-space-100); | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-bottom:empty) { | |
| top: 50%; | |
| } | |
| /* | |
| * The vertical-top badges are anchored to the tab-button's top edge, so | |
| * when the icon shifts down with the margin above, they drift visually | |
| * away from the icon. Re-anchor them so they continue to overlap the | |
| * icon's top edge. | |
| */ | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top) { | |
| top: 2px; | |
| } | |
| :host(.tab-layout-icon-top.tab-has-icon.tab-has-label) ::slotted(ion-badge.badge-vertical-top:empty) { | |
| top: 0; | |
| } |
What is the current behavior?
Tab buttons in the ionic-md theme are slightly shorter than the tab bar. This is inconsistent when dev tools are in use in Firefox, causing screenshot tests to fail.
What is the new behavior?
Does this introduce a breaking change?
Other information
The tab bar content height is 48px while the buttons are 44px high with the styles specified. Usually the buttons render 44px high, but sometimes when the button is focused with Firefox dev tools, they render 48px high. When the buttons are made to be 48px high, matching the container, we don't see this issue.