Fix Nav Block to allow overflow scroll when there are many horizontal nav items #18336

getdave · 2019-11-06T16:53:31Z

Currently, in the new Nav Block, if many nav items are created then it forces the editor canvas to "expand" and it is broken.

This PR addresses this by allowing overflow to be controlled by scrollbars when the number of items exceeds the width of the Nav Block within the Editor canvas.

This is only applied to the Block within the Editor.

Questions/Problems

There is a question for @karmatosed and @shaunandrews regarding if/how we indicate the presence of "off-screen" items. For example if you have a lot of items yuo have to start scrolling to know they are there.
What happens when the block appender is hidden offscreen because it's always on the far RHS of the items.
~~You will notice if you focus the rightmost item then the toolbar is cut off. Could/should we add some extra spacing to the far RHS of the Block when it is focused in anticipation of this happening?~~ I've added RHS spacing of 25% only if there is hoz scroll.

How has this been tested?

Enable Nav Block in experiments.
Add a Nav Block with loads of Nav items.
See the Nav Block does not expand the Editor canvas but rather shows hoz scrollbars.
You should be able to access all Nav Items as before without the Editor looking "broken".

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.

Now detects hoz scroll and adds suitable class which can be used to hook CSS styles for when there is hoz scrolling.

retrofox · 2019-11-07T12:15:43Z

It breaks the toolbar in my testing site:

Is it just me?

retrofox

I'm requesting changes in order to improve the performance of this approach, Dave.

Let's try to conditionate the effect call passing the scroll and client with values, something like the following:

const clientWidth = navMenuRef.current ? navMenuRef.current.clientWidth : 0;
const scrollWidth = navMenuRef.current ? navMenuRef.current.scrollWidth : 0;

useLayoutEffect( () => {
    //...
}, [ scrollWidth, clientWidth] );

It will reduce considerably the times that the callback function is called.

obenland · 2019-11-07T22:26:47Z

It breaks the toolbar in my testing site. Is it just me?

Happens here, too. Introduced by the overflow-x rule.

Addresses #18336 (review)

getdave · 2019-11-08T10:27:26Z

It breaks the toolbar in my testing site:
Is it just me?

@obenland @retrofox Unfortunately this looks like a major problem which stems from

having toolbars for Blocks which are "pulled" outside their containing element
needing an overflow on the parent Block
using nested Blocks within a scrolled parent

Because we add overflow-x: auto to the Nav Block (parent) it clips the layout box of the Nav Block Item (child).

Why does this happen on the Y when we're only setting X to auto?

Because the spec doesn't allow certain combinations of X and Y..

In the case that X is auto then Y automatically assumes the same value. So Y becomesauto (despite not being explicitly set!) which means that the toolbar is clipped because the Y overflow is now controlled by scroll rather than allowed to "overflow" outside the box.

The only way I can find to fix this is to add an inner element and set the overflow on that. That alone doesn't solve things because we've just moved the problem down 1 level. So we need to create enough space for the Nav Block Item (child) toolbar to be visible. To do this we adding top padding to the inner element and then cancelling that out visually using a negative margin. This creates an artificial space for the toolbar to display within whilst still preserving the scroll behaviour.

The screenshot below shows this in action:

This feels like a horrible hack but it seems to be the only way to make this work. That said I'd appreciate 👀 from everyone else to make sure I'm not overlooking a much simpler solution.

Alternative Route

Another way around this might be that we proxy the child Block's toolbar to be shown by the parent Block. Personally I don't find having individual "inline" toolbars for all the sub menu items very usable. Perhaps if we mirrored something like the behaviour of the Table Block where the toolbar remains fixed to the parent Block and clicking into individual cells causes that Toolbar to update based on context. I will highlight that Table does not use child Blocks so this is not the same use case. However we could consider this.

Pinging @karmatosed and @shaunandrews for input here and also @jasmussen because I know he's the person to talk to when it comes to Block layout within the editor.

jasmussen · 2019-11-08T10:52:12Z

Yes, this is a doozy. I doubt we can leverage any kind of overflow rule to solve this, and not just for the technical margin/padding reasons mentioned, but because for a user to have to scroll horizontally to reach the inserter is not a good experience.

With autohiding scrollbars it's unclear there's anything to scroll. With permanently visible scrollbars, it's an inaccurate representation of the menu on the front-end.

Which brings us to the relationship between editor and front-end — what overflow handling, if any, do we have on the front-end? And can we bring that overflow handling to the editor as well, even if only visually? For example if on the frontend overflow is handled by a dropdown arrow that appears when there are too many menu items, perhaps the very same dropdown arrow could appear in the editor.

As for how to then add menu items, I would use this one:

While it might need a little more work to be able to handle menu creation 100%, it feels like the only scalable solution for insane mega menus.

All of this still follows the 80/20 rule. Most people will never encounter overflow and be fine to create top level items. But for the 20% that do — we should avoid needing to scroll horizontally, vertically, or to hunt for inserters.

What do you think?

getdave · 2019-11-08T15:51:44Z

Thank you so much for your detailed response @jasmussen. Really helpful.

Agree with what you say about the UX and also the 80/20 rule.

That said, this PR/Issue is currently marked as blocking the MVP of the Nav Block. Therefore I'm interested in whether we can devise an acceptable interim solution?

I'm not entirely comfortable with my current hack solution. However, allowing the editor to visual "break" (as in clearly in an unintended, visual error state) when people add too many items is also sub-optimal.

One option is to allow items to wrap rather than extend indefinitely to the RHS. This would be easily to map between frontend and editor. Moreover, from a UX perspective it would be easy for a user to perceive that there are too many items in the Nav and that they need to refine their options.

I think we should keep in mind that it's not best practice to have too many top-level nav items. So by clearly indicating a non-optimal visual state (but which at the same time isn't obviously "broken") we clue the user to consider their options.

The question is by making the implementation too resilient are we doing the user a disservice?

I do see the Block Navigator in the inspector so I assume this is still a key piece of UI. However, I think the question of direct Block manipulation might be too large to address in this PR.

jasmussen · 2019-11-11T06:27:09Z

Therefore I'm interested in whether we can devise an acceptable interim solution?

One option is to allow items to wrap rather than extend indefinitely to the RHS.

Absolutely, and actually wrapping might be a good such interim solution.

This would be easily to map between frontend and editor. Moreover, from a UX perspective it would be easy for a user to perceive that there are too many items in the Nav and that they need to refine their options.

There's one additional principle we need to keep in mind here. While the unselected block should be as close to a front-end preview as possible, the selected block does not have to map. I.e. we could move forward in two ways:

We wrap the menu items only when the nav block is selected, and handle overflow with a ellipsis dropdown shown when unselected. Needs the frontend ellipsis menu implemented.
We wrap the menu items front and backend and call it a day.

1 seems like a nice navigation menu toggle option to have.

2 seems like a good improvement, and one that is potentially easy to implement.

1 is the only of those two that has some kind of a mobile strategy, which seems important for the MVP nav block (but maybe not blocking?). But I'm not sure that's the intent of the original ticket, so that seems fair to track and look at separately.

In other words, 2 seems totally fine for me, for starters.

Addresses #18336 (review)

Previously there were many issues with the editing experience and UI when allow for overflowing editing. Experiment with allow items to wrap when the Block is selected. See #18336 (comment)

getdave · 2019-11-11T12:03:38Z

@jasmussen I'm now trying the wrap approach.

tellthemachines

I left a few comments below for simplifying this change. One other thing I noticed is that the layout area is still getting a horizontal scroll when the last item in the nav is focused:

Not 100% sure why this is happening but I think it might be related to the fact that the appender at the end of the nav is hidden by default, and only shows when that last item is focused. This behaviour might have changed in the meantime because it doesn't happen on the master branch.

tellthemachines · 2019-11-13T05:41:37Z

packages/block-library/src/navigation-menu/edit.js

@@ -79,6 +95,7 @@ function NavigationMenu( {
 		'wp-block-navigation-menu', {
 			'has-text-color': textColor.color,
 			'has-background-color': backgroundColor.color,
+			'has-scroll-x': hasScrollX,


This classname doesn't seem to be used for anything. If we can scrap it we can also get rid of all the state and effect logic above.

tellthemachines · 2019-11-13T05:43:16Z

packages/block-library/src/navigation-menu/edit.js

@@ -148,15 +165,17 @@ function NavigationMenu( {
 			</InspectorControls>

 			<div className={ navigationMenuClasses } style={ navigationMenuInlineStyles }>
-				{ isRequesting && <><Spinner /> { __( 'Loading Navigation…' ) } </> }
-				{ pages &&
+				<div ref={ scrollContainerRef } className="wp-block-navigation-menu__scroll-container">


The scroll container is unnecessary; the styles can just as easily be applied to wp-block-navigation-menu.
Also, it seems to be causing small navs to get a horizontal scrollbar:

tellthemachines · 2019-11-13T05:47:28Z

packages/block-library/src/navigation-menu/editor.scss

+		// value clipped (see above). This value is equal to that of the positioned
+		// toolbar element (usually `top: -51px`).
+		padding-top: 51px;
+		margin-top: -51px;


Might be nice to add padding-left: 17px here to avoid the toolbar on the first nav element being cut off:

getdave · 2019-11-13T09:16:40Z

Noting that this PR will become a lot simpler when https://github.com/WordPress/gutenberg/pull/18440/files lands.

jasmussen · 2019-11-13T09:21:35Z

Wanted to note that I'm still skeptical that it's a good idea at all to use a scrollbar to fix this problem. It is inconsistent with the front-end even when unselected, which is sort of the prime directive of the block editor.

The wrapping approach does not have that problem.

shaunandrews · 2019-11-13T19:31:20Z

A scrollbar feels clunky. Perhaps we can use a more established pattern on the web, and show a more button to show overflowing items. Here's how something like that could look/work:

karmatosed · 2019-11-13T21:53:40Z

My only worry with the more is we are changing the '+' icon into that. It feels weird to change such a focal existing interaction that is used there. Maybe a '...' after?

jasmussen · 2019-11-14T08:11:39Z

Responded to Shaun's comment in #18429 (comment) but it applies here as well:

But I wonder if the interface for editing the menu should just let items wrap, as this 3 line PR does: #18431. Picture this — you select the block to edit it, and items wrap. You deselect it and overflowing items are moved into a dropdown menu.

The benefit of that approach is that we can merge #18431 as is to fix #18298, and then we can followup with a PR to enhance that further.

getdave · 2019-11-14T13:48:37Z

Closing in favour of Wrap solution. See #18298 (comment)

getdave added 2 commits November 6, 2019 16:48

Fix to allow overflow scroll when many horiztonal nav items

5ba0ba5

Limit to x axis only

de302bb

getdave requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners November 6, 2019 16:53

getdave self-assigned this Nov 6, 2019

getdave added [Block] Navigation Affects the Navigation Block [Package] Block library /packages/block-library labels Nov 6, 2019

getdave requested review from shaunandrews, karmatosed, retrofox, frontdevde, obenland and marekhrabe November 6, 2019 16:59

getdave added 2 commits November 7, 2019 09:17

Updates to make space on RHS for item block toolbars and block appender

4a1f457

Updates to only add RHS padding if scroll is present

e5b4ab8

Now detects hoz scroll and adds suitable class which can be used to hook CSS styles for when there is hoz scrolling.

retrofox requested changes Nov 7, 2019

View reviewed changes

getdave added 2 commits November 8, 2019 09:41

Refactor to optimise scroll detection

582de84

Addresses #18336 (review)

Fixes clipped scrollbar using artifical padding technique

01383ce

getdave mentioned this pull request Nov 8, 2019

Navigation block item breaks on mobile #18401

Closed

getdave added a commit that referenced this pull request Nov 11, 2019

Refactor to optimise scroll detection

ca4261a

Addresses #18336 (review)

getdave mentioned this pull request Nov 11, 2019

Nav block: handle overflowing items using wrap on Block selection #18429

Closed

6 tasks

getdave mentioned this pull request Nov 11, 2019

Experimental: allow parent Block to consume child Block's toolbar #18440

Merged

6 tasks

tellthemachines requested changes Nov 13, 2019

View reviewed changes

getdave mentioned this pull request Nov 14, 2019

Navigation Menu: handle overflowing Nav Items #18298

Closed

getdave closed this Nov 14, 2019

aristath deleted the fix/nav-block-overflow-scroll branch November 10, 2020 14:21

getdave mentioned this pull request Jan 20, 2021

Navigation block: Hide more than N items #27131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Nav Block to allow overflow scroll when there are many horizontal nav items #18336

Fix Nav Block to allow overflow scroll when there are many horizontal nav items #18336

getdave commented Nov 6, 2019 •

edited

Loading

retrofox commented Nov 7, 2019

retrofox left a comment

obenland commented Nov 7, 2019

getdave commented Nov 8, 2019 •

edited

Loading

jasmussen commented Nov 8, 2019

getdave commented Nov 8, 2019

jasmussen commented Nov 11, 2019

getdave commented Nov 11, 2019

tellthemachines left a comment

tellthemachines Nov 13, 2019

tellthemachines Nov 13, 2019

tellthemachines Nov 13, 2019

getdave commented Nov 13, 2019

jasmussen commented Nov 13, 2019 •

edited

Loading

shaunandrews commented Nov 13, 2019

karmatosed commented Nov 13, 2019

jasmussen commented Nov 14, 2019

getdave commented Nov 14, 2019

Fix Nav Block to allow overflow scroll when there are many horizontal nav items #18336

Fix Nav Block to allow overflow scroll when there are many horizontal nav items #18336

Conversation

getdave commented Nov 6, 2019 • edited Loading

Questions/Problems

How has this been tested?

Screenshots

Types of changes

Checklist:

retrofox commented Nov 7, 2019

retrofox left a comment

Choose a reason for hiding this comment

obenland commented Nov 7, 2019

getdave commented Nov 8, 2019 • edited Loading

Alternative Route

jasmussen commented Nov 8, 2019

getdave commented Nov 8, 2019

jasmussen commented Nov 11, 2019

getdave commented Nov 11, 2019

tellthemachines left a comment

Choose a reason for hiding this comment

tellthemachines Nov 13, 2019

Choose a reason for hiding this comment

tellthemachines Nov 13, 2019

Choose a reason for hiding this comment

tellthemachines Nov 13, 2019

Choose a reason for hiding this comment

getdave commented Nov 13, 2019

jasmussen commented Nov 13, 2019 • edited Loading

shaunandrews commented Nov 13, 2019

karmatosed commented Nov 13, 2019

jasmussen commented Nov 14, 2019

getdave commented Nov 14, 2019

getdave commented Nov 6, 2019 •

edited

Loading

getdave commented Nov 8, 2019 •

edited

Loading

jasmussen commented Nov 13, 2019 •

edited

Loading