Make top bar accessible when zoomed in.

[tellthemachines]

Description

Partially fixes #15346

Top bar should wrap at high zoom levels and small breakpoints.
Table block should scroll horizontally when its contents are wider than the screen, unless "fixed table width" is set.
Words inside table cells should not break unless "fixed table width" is set.

UPDATE: Reverted table changes on this PR and will do a separate PR for them.

How has this been tested?

Manually tested on Chrome/Mac, Safari and IE at 400% zoom; also checked on iPhone simulator.
Ran unit and e2e tests locally.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[jasmussen]

I would like for us to get @ellatrix input on this one before we ship. I'm almost certain that I've explored something similar back in the past in efforts to make the table responsive, but every solution I found had some breaking downsides.

[ellatrix]

I forgot the details as well. Isn't table the default display value in case of a table element? Why only for aligned tables? This should receive an inline comment.

[tellthemachines]

Yup, sorry, I should have described the changes better. I'm setting the default display for the table to block so that, with overflow-x: scroll, it can scroll horizontally on small breakpoints, as recommended in the issue. However, for fixed-width tables and for aligned tables that won't work, so I'm resetting the display value to table for those. I'll put a comment in to make it clear.

[tellthemachines]

Done!

[afercia]

For future reference: it is important to note that some CSS display properties do affect table semantics. For example, flex | block | grid | inline, they all override the table native semantics, rendering it useless to a screen reader. Keeping the native display: table is the right way to go for data tables, thanks for that! 🙂

[jasmussen]

We should always be careful when using the vw unit, because it does not account for the presence of visible scrollbars. So using 100vw might unintentionally cause a horizontal scrollbar.

[tellthemachines]

Good point. I checked Chrome and Safari on Mac, Chrome and IE on Windows 7 and Windows 10, and it's not happening, though likely that's because we have overflow-x: hidden on the body element for screens under 782px. If we removed that, we'd get a massive horizontal scroll because of the admin bar having a min-width of 300px. That, and a couple other min-widths set on the core side, are the reason I had to resort to vw in the first place, to get the top bar to not overflow the viewport.
I'll check trac to see if there's a ticket for addressing that, and create one if there isn't, but suspect it will be non-trivial to fix on that side 😅 so for now we're probably better off with the vw solution.

[jasmussen]

Wow, impressive work, thanks so much for thinking of, what in this case is beyond mobile. At a glance, the changes seem okay, if things things we want to both test very thoroughly.

I left a few comments in the code, but overally my primary question is how well this has been tested on non mobile, and very wide breakpoints. Due to the way the CSS and responsive mixins are structured, the CSS is written mobile first and then overridden on ever-increasing breakpoints. Which means the bulk of the CSS written here applies across every breakpoint, because there's nothing to "unset it". This may absolutely be intentional, but it also makes this PR a little more sensitive because it touches the layout of every breakpoint out there, necessitating a need for quite a bit of browser testing.

[tellthemachines]

Thanks for the feedback @jasmussen! I have now checked the large/very large breakpoints on both Mac and Windows (on an actual laptop running Windows 10, and on a virtual machine running Windows 7), across Chrome, Firefox, Safari and IE, and also iPad pro with Safari on the Xcode simulator, and they're all displaying fine.

[kjellr]

Hey @tellthemachines! In general, this looks excellent. Thank you for diving into this.

I tested on a Mac, with scrollbars set to "Always", and have a little bit of feedback. When a table is brand new, it used to take up the entire width like this:

Is there any way we can preserve that? The PR currently presents me with this:

This looks a little broken, and it's also weird that it's showing me disabled scrollbars by default. I wonder if there's a way to eliminate those?

[tellthemachines]

Oh, good spot @kjellr! The scrollbar can be fixed by setting overflow to auto instead of scroll. The empty cells not taking up full width is due to the display on the table element being set to block to allow scrolling, and that doesn't play well with the default display modes of the children. I'll investigate if there is any CSS-only workaround for this, otherwise we might have to go back to using a div wrapper for the table block. That's a larger piece of work though, and I was hoping we could get away without any markup changes. 😅

[tellthemachines]

Update on this: I can't find a way of making the empty table go full width with only CSS, so am going to go the wrapper div route. I have another branch with most of that work done already so will remove the table styles from this PR and do a separate one for the div wrapper. This PR should then only address the top bar issue.

[jasmussen]

Thank you for your continued work on this.

I know it's just a few lines of code, but they also happen to be fundamental and structural to the entire layout. Unfortunately a few things broke in the process, that we'll have to fix first.

The header bar is now taller, and items inside misaligned. Before:

After:

I left a comment in context of the code about the change to position: sticky; for mobile. As noted, this is a topic that can be discussed, but if it isn't crucially necessary for this PR, I would suggest exploring it separately, as it's a big change.

The columns block broken entirely. This branch:

Master:

Fullwide alignments broke. This branch:

Master:

I'm almost certain it's the vw unit width, which is not aware of neither left hand navigation sidebars, right hand settings sidebars, or even scrollbars in either of the three. If there is any way at all we can avoid the vw unit, that would be preferable as it can theoretically be the same code across all breakpoints and zoom levels leading to better code quality. If the vw unit truly is the only way forward, it needs to be unset beyond the mobile breakpoint.

I understand this is a priority and I deeply appreciate your work here, but I believe the best way forward is to change as little code as possible, in as small a PR as possible. I know it's already pretty small, but some things to try:

• Use min-height instead of removing the height and moving it to the mobile+ breakpoint.
• Instead of using flex shorthand, keep the specific flex properties as they are, but remove or add any non-shorthand additional flex properties as needed.
• Avoid vw if at all possible. If 100% works, that will likely cause fewer cascading effects.
• Avoid changing the sticky unless it's necessary.

Thanks again.

[tellthemachines]

The misalignment of the top bar was well spotted @jasmussen ! That was actually due to the flex-wrap I set on edit-post-header__settings. I have fixed it now.

Regarding the issue with the horizontal scroll on the full width image, that was an unrelated regression and was fixed in #16295. I have rebased master so the fix should now be on this branch.

The columns issue is a funny one. Before rebasing, I was seeing the previous version of the block, where creating a new columns block immediately brings up a default two-column layout:

After rebasing, I now see the placeholder displaying correctly on my branch, but I was able to reproduce the issue on your screenshot by clicking the sidebar. That behaviour is unrelated to my changes though, and I can reproduce it on master, so I have created issue #16343 for it.

[jasmussen]

Gosh, it sounds like a lot of those issues were indeed rebase related. Thanks yet again for your continued work on this.

I am about to be AFK for the day, so I can't verify everything works as intended. I also maintain that we should be exceptionally careful about using the vw unit, and if you can use percent instead, it would very probably be more resilient code for it. Did you have any thoughts on this?

If someone else has the time to code review this and approve it before I'm able to test, they should feel free to dismiss my change requests if they are addressed.

[tellthemachines]

Haha @jasmussen I'm just now replying to your question on my other PR about the vw issue. No worries, hopefully we can sort this out soon 😄

Dismissing these as the changes requested are addressed.

[jasmussen]

In testing this, everything seems to work as the PR intends. None of the previously mentioned issues are present anymore.

The editing canvas isn't usable at all at this zoom level:

... but I imagine that is separate from this PR, especially given the offending overflow-x is previous core code.

The vw unit is still added as a max-width, which as I've noted in the other PR feels like the wrong approach. But our very level-headed friend, @kjellr gave some great thoughts on #16255 (comment), and I imagine the same thoughts apply here. So I've dismissed my own change request, and if Kjell can confirm the same thoughts apply here, we can merge this right in. Thanks again for your work.

[kjellr]

This variable name reads a little awkwardly to me — should it bebreak-min-zoom or something? Or perhaps we just need to add a dash in, so that it reads as break-zoomed-in. Interested in what @jasmussen thinks.

[jasmussen]

Great thoughts. Both your suggestions are nice.

[tellthemachines]

Done! Going with zoomed-in so it's obvious what the use case scenario is.

[kjellr]

The vw unit is still added as a max-width, which as I've noted in the other PR feels like the wrong approach. But our very level-headed friend, @kjellr gave some great thoughts on #16255 (comment), and I imagine the same thoughts apply here. So I've dismissed my own change request, and if Kjell can confirm the same thoughts apply here, we can merge this right in. Thanks again for your work.

Agreed, I think this is another one of those circumstances. Like the other PR, we should add a code comment referencing the Trac issue. @tellthemachines, would you mind adding that in?

Aside from that and a minor variable name question I raised above, I think this is ready to go. It looks like the ellipsis menu is basically unusable at super-high zoom levels, but that's a different PR. 😄

Thanks for the work here, @tellthemachines!

[kjellr]

This is working well in my tests. Thanks for all the work on this, @tellthemachines!