Navigator: Remove overflow styles from NavigatorScreen

[youknowriad]

Extracted from #44770

What?

This PR reverts a change introduced in #35518 This code has been introduced to make sure sticky positioning work inside the navigation screens. In my testing (fortunately, we have a storybook story to confirm the behavior), the intended sticky behavior is still working and the "overflow" rule was causing issues for "focus styles" of buttons that may appear at the edge of screens. They would appear cut.

Testing Instructions

1- Confirm the sticky behavior still works in the "preferences modal" on mobile

[jorgefilipecosta]

LGTM 👍
The sticky behavior seems to work well when testing on a small window size with the preference mode ( I don't see a different when compared to the trunk).

[stokesman]

Excuse my tardiness on this. I'm not seeing a good reason to remove these rules. While I wouldn't say they are crucial, they are meant to safeguard and facilitate third-party use of the component. Their introduction stemmed from this concern of unreachable content from #35317 (comment)

scenarios where the consumer inadvertently causes x-overflows with naive CSS and dynamic content (translations, tables, etc).

Removing these rules did break the part of the Navigator story meant to demonstrate such scenarios. Check the screen behind this button:

With regards to the clipping issue:

the "overflow" rule was causing issues for "focus styles" of buttons that may appear at the edge of screens. They would appear cut.

The way to avoid that is putting the padding inside the screen (Example) That would have been the easy fix for clipped focus indicators in #44770. (I say would have because it no longer seems to be relevant as I cannot see that NavigatorScreen is even used in the PR.) We'd probably want to have guidelines about this as part of the Navigator documentation.

So I'm voting to revert this but only if the components folks @mirka and @ciampo still think the rules are worthwhile.

[youknowriad]

The navigator usage moved to this PR #45100
I know that when I used it initially, adding padding was not an option, it might be know, worth checking.
But personally I still feel like it might not be right to have these rules in the screen component since it's not meant to be opinionated in terms of design. Not a strong objection though.

[stokesman]

Thanks for the link to that PR. I've left some feedback on it #45100 (comment) that might help clarify why it makes sense to always keep the padding on the inside of the component. It's also worth noting that even with these rules removed that PR still exhibits the clipped focus indicators because NavigatorProvider has overflow-x: hidden since #35332 which stemmed from the issue of horizontal scrollbars appearing during animation (#34904 (review))

While I agree with the idea of the component having minimal opinions on design I think these rules follow from the big opinion it does have—that the screens animate horizontally.

Now after all that I've realized there is one way we could remove all overflow rules from these components. That would be to leave it up to implementors to wrap them within a container that has overflow: hidden or add that rule to their component instance, or stop worrying and love the appearance of horizontal scrollbars during animations.

[ciampo]

Removing these rules did break the part of the Navigator story meant to demonstrate such scenarios. Check the screen behind this button:

I can confirm that I can spot this regression too.

While I agree with the idea of the component having minimal opinions on design I think these rules follow from the big opinion it does have—that the screens animate horizontally.

Allowing overflow IMO is not really an opinionated design choice, but we may indeed argue if we should expect the consumer of Navigator to add those styles externally instead (WDYT, @mirka ?)

---

More in general, I just wanted to point out that this PR originated from my request to split Navigator changes to a separate PR, so that we could discuss them and review them in isolation. But this PR was reviewed and merged in less than 4 hours from its opening, making it impossible for other folks to have a conversation around these changes. In the future, I'd love if we could avoid merging PRs in such a rush. Thank you!

[youknowriad]

I'm happy to revert this PR if you all think it's the way to go.

that we could discuss them and review them in isolation. But this PR was reviewed and merged in less than 4 hours from its opening

I'm very uneasy with this kind of comment. I really don't agree that 4 hours is "rushing" for a small PR like this. Sure regressions can happen and reverting is very easy, we just need to be responsible for follow-ups but I'm very uneasy with gate keeping.

[ciampo]

I'm very uneasy with this kind of comment. I really don't agree that 4 hours is "rushing" for a small PR like this. Sure regressions can happen and reverting is very easy, we just need to be responsible for follow-ups but I'm very uneasy with gate keeping.

First of all, I'm sorry if my comment made you feel uneasy — it was definitely not my intention.

From my point of view, it's really not about the size of the PR, but more about allowing folks directly involved (@stokesman and myself) to chime in with their opinion and review the PR. 4 hours isn't a long time, considering that we may have to take care of other high-priority tasks first, be based in another timezone (like Mitchell is) or simply we could have an unexpected matter to attend.

Ultimately I believe that my previous comment was trying to avoid gate-keeping in the first place, by expressing the need for more discussion before merging.

I'm happy to revert this PR if you all think it's the way to go.

If the purpose of the PR was to prevent clipping on the focus indicators, I still believe that the problem is not addressed. As @stokesman points out, the main cause for this is probably overflow-x: hidden style on NavigatorProvider:

gutenberg/packages/components/src/navigator/navigator-provider/component.tsx

Lines 86 to 87 in 6bc1ec5

	// Prevents horizontal overflow while animating screen transitions.
	() => cx( css( { overflowX: 'hidden' } ), className ),

)

Those styles would be more problematic to remove, since they prevent scrollbars from showing during a transition (see #34904 (review)).

The simpler way to avoid clipping, as Mitchell points out, is to add padding to the contents of NavigatorScreen.

Regarding the overflow-x: visible and max-height: 100% rules that this PR deleted:

• the max-height: 100% rule seems reasonable to me, as it makes sure that the NavigatorScreen doesn't exceed its root container's height.
• the overflow-x: visible rule is a more subjective/opinionated manner. I think it's ok to expect that consumers of NavigatorScreen would add the extra CSS rule to the component when necessary

What do folks think?