Try: Improve scrolling of navigation menu on small screens

[jasmussen]

We scroll the editing canvas, the inspector sidebar, and the block library independently at desktop breakpoints. We do this so that the sidebar inspector can stay in view even if you have scrolled far down the editing canvas, and to avoid scroll bleed.

However because the navigation sidebar (on the left) has flyout menus on the desktop breakpoints, we can't yet scroll this one separately. If a user has a bunch of plugins installed that add menu items, or a small screen, these manu items might go beyond the visible height of the viewport. To make these accessible regardless, when this happens the body element scrolls to let you reach them.

In this situation, there is currently an issue where the editing canvas might scroll out of view when you scroll to the bottom of the sidebar.

This PR improves that situation by making the editing canvas position: fixed;, same as the sidebar is. This ensures that the entire editor scrolls with you down the page, as you scroll the body content.

This needs a good testing. position: fixed; does not inherit from a relative parent, which means we have to specifiy a matrix of left margins to accommodate for the different configurations of the navigation menu: auto-collapsed, manually collapsed, or the default width. To test, please verify that everything works as intended. Please test:

• All breakpoints beyond the 600px small breakpoint.
• Fullscreen and not fullscreen modes.
• With the navigation menu auto collapsing, and explicitly collapsed.
• With the inspector sidebar present or hidden.
• With metaboxes present and not present.

Before:

After:

[chrisvanpatten]

Oh @jasmussen you're my hero — this has bugged me for a while but I have been too busy to wade in. Planning to give this a test in the next day or so!

[youknowriad]

Just a note to make sure we test with meta boxes too (and IE) :)

[jasmussen]

Rebased, and did further testing. Edge and IE look fine. Edge:

IE:

Note, IE has some weird overlaying autohiding scrollbars which we can do nothing about, but this is unchanged in this PR and simply how IE11 works.

Discovered a small metabox issue I'm investigating, so this is more of a progress report. Nope, there wasn't an issue. I was testing without the Gutenberg plugin enabled ¯_(ツ)_/¯

[jasmussen]

There's an e2e test that keeps failing, but I don't understand why as it seems entirely unrelated:

 FAIL  test/e2e/specs/writing-flow.test.js (14.037s)
  ● adding blocks › Should navigate inner blocks with arrow keys

    expect(received).toBe(expected) // Object.is equality

    Expected: "First column paragraph"
    Received: "Second column paragraph"

      50 |              await page.keyboard.press( 'ArrowUp' );
      51 |              activeElementText = await page.evaluate( () => document.activeElement.textContent );
    > 52 |              expect( activeElementText ).toBe( 'First column paragraph' );

[jasmussen]

Further testing with metaboxes present. Chrome:

Firefox:

Safari:

Edge:

IE11:

☝️Bummer — see the metaboxes overlapping. That rings a bell. But as it turns out, this is also an issue in master:

Turns out IE11 is so bad it renders as if the browser is in quirks mode. I will look for an IE11 hack.

[jasmussen]

Okay, found a rather delightful and simple fix (478e9bc) that makes the flexing work in IE11. Here's a swath of new testing. IE11:

Edge:

Chrome:

Firefox:

Safari:

The iphone simulator for good measure:

I feel pretty good about this one. But would still appreciate your sanity checks, but also advice on why the checks are failing.

[tofumatt]

This looks complicated but aside from a few tweaks the SCSS looks fine to me. Thanks for all the comments.

[tofumatt]

What is being undone here? Could you clarify what "this" is? 😄

[tofumatt]

These comments after each attribute get really long and make for quite long line-lengths. If they could be moved around that'd be grand, but having them relate to the attributes is handy too, so no worries if you wanna leave 'em as-is.

[chrisvanpatten]

Looks good from a UX perspective for me. The test fails might be fixed by a rebase, they seem unrelated. I can't get e2e tests working locally so I can't test, unfortunately.

[tofumatt]

It might be worth rebasing on top of master, I think the tests were a bit odd for awhile after the 5.0 release? Maybe a rebase’ll fix it. *fingers crossed*

[chrisvanpatten]

I rebased and pushed but Travis is still failing. However the failure was that phpcs couldn't be found, so I'm inclined to think it's an issue in Travis. Restarting the tests and crossing my fingers 🤞

[youknowriad]

Another rebase here should fix the tests.

[jasmussen]

Rebased, hoping the tests will pass.

[youknowriad]

It seems that the test failure is legit here. (the writing flow failure). Would be good if someone could take a look @WordPress/gutenberg-core

[jasmussen]

Yep, I'm not sure why a change in scrolling behavior should cause the test: #12644 (comment)

Would deeply appreciate some insights. 💕

[youknowriad]

Yep, I'm not sure why a change in scrolling behavior should cause the test: #12644 (comment)

This could have a small impact on the width of the columns for instance, and this means maybe while previously we had to do two arrow ups to move to the first row of the block, we need to do only one at the moment.

[jasmussen]

This could have a small impact on the width of the columns for instance, and this means maybe while previously we had to do two arrow ups to move to the first row of the block, we need to do only one at the moment.

Makes sense, and I could probably "fix the test" to make it pass. But I wouldn't necessarily fully understand what I did in the process, and I would rather if someone who was involved with the original test, or understands it better than I do, help give insight, as I'm worried about breaking the tests by "fixing" them.

[youknowriad]

There's an issue related to this change I think that cause the columns block to be very tight in the e2e tests. The left position of the editor is not correct I think.

Instead of duplicating all the .folded, .auto-fold... styles, can't we use the .editor-left mixin as I think it's meant to address exactly this issue.

[jasmussen]

We probably could, yes, but the mixin sets the left property, whereas we have to set the margin-left property.

It's complicated to explain why we need to use margin instead of the left property, but the reason is the same as why the sidebar uses it. The sidebar is fixed position using the same method as now the editing canvas, and by using margins we don't have to account for whether a scrollbar is present or not.

For example the notices use editor-right, and for that reason they overlap scrollbars, which is one of the things that were fixed in #12301. You can see that the sidebar also uses margins for that reason, here: https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/components/sidebar/style.scss#L114

I suppose I can create a new mixin, @editor-margin-left that is similar to @editor-left, do you think that would be a good approach?

[youknowriad]

I don't know, if it's specific to this component maybe not worth-it. But it would be good to detect why the margin-left is wrong in the e2e tests.

We're probably missing a case here.

In the e2e test, I see the WP left menu as collapsed but the editor toolbar and the editor content left position is wrong, it's shown as if the left menu is not collapsed.

[jasmussen]

Let me take a look again to see if the rules are right.

[jasmussen]

Just tried to follow the instructions in the test all manually, and they fail for me too. However they fail the same for me in master and in this branch. It's only the e2e test that fails in this branch:

I'm really confused why this test fails.

I've tested thoroughly the left margin now, and I cannot get it to a state where it isn't right, with regards to the state of the left menu.

[jasmussen]

In the e2e test, I see the WP left menu as collapsed but the editor toolbar and the editor content left position is wrong, it's shown as if the left menu is not collapsed.

How did you get to this state? I can't reproduce it.

[youknowriad]

The thing is I couldn't reproduce as well unless I run the e2e test in non-headless mode.
I think it has to do with:

• First time you visit the admin (no persisted preference for the sidebar...)
• Size of the window should be something around 960x700

[jasmussen]

Awesome, thank you. Trying.

[jasmussen]

Oh wow this is super interesting. At that breakpoint, the text "Second column paragraph" wraps into two lines:

That means that you do need THREE presses of the up arrow to go to the 1st column. Whereas when there's enough space in the main editing canvas for the text to not wrap, you only have to press the up arrow TWICE.

Regardless of the test failure, or this patch I'm currently testing, this means the test currently only works on a very specific screen size.

[jasmussen]

So, here's a GIF of me reproducing master at about that screen size:

Here's me manually reproducing the same test in this branch:

The bit that is failing is that once the caret is at the end of the second column paragraph, you press "Up" arrow three times. One to go to the first line, another to select the column, and a third to go to the 1st column paragraph.

This is the same in both branches when I test.

But if the viewport is this small, then the words "Second column paragraph" wrap onto three lines:

If that were the case, then it would produce the expected result from our test failure, i.e. return "Second column paragraph", because the 2nd column would be selected.

[jasmussen]

I found a fix.

Turns out in my IE11 fix here, I had prevented the editor from shrinking, meaning the metaboxes were outside the visual area on an empty screen. The width of the scrollbar this caused, meant the text from the test, "Second column paragraph" wrapped onto THREE lines instead of TWO.

So the test, in a roundabout way, worked. I expect the tests to pass now.

ce7dfe0 includes some feedback from @tofumatt addressed, and it also changes the matrix of the folded/autofold rules to better match that from the mixin. But the net result for that change is identical to before. The real change is here: ce7dfe0#diff-ec5f5c91d0b3295f448977820605e4e4R147

That changing of flex means it works for both IE11 and other browsers.

Before, it wasflex: 1 0 auto;, which means it can flex-grow, it can't flex-shrink, and it has an auto size.

Now it's flex: 1 1 auto;, which means it can flex-grow, flex-shrink, and has an auto size. The "flex shrink" means there will be no scrollbar when the test is run. This means the text won't wrap, and means the test should return the right result. 🤞

[jasmussen]

Okay, it appears the test is still failing. Which puts me at the end of my rope. The first test failure was legitimate in that I discovered the missing "flex-shrink" property that caused metaboxes to be out of view.

But I don't know why it's failing now, even locally.

Changing the test to the following makes it pass:

	it( 'Should navigate inner blocks with arrow keys', async () => {
		let activeElementText;

		// Add demo content
		await clickBlockAppender();
		await page.keyboard.type( '1st paragraph' );
		await page.keyboard.press( 'Enter' );
		await page.keyboard.type( '/columns' );
		await page.keyboard.press( 'Enter' );
		await page.keyboard.type( '1st column paragraph' );

		// Arrow down should navigate through layouts in columns block (to
		// its default appender). Two key presses are required since the first
		// will land user on the Column wrapper block.
		await page.keyboard.press( 'ArrowDown' );
		await page.keyboard.press( 'ArrowDown' );
		await page.keyboard.type( '2nd column paragraph' );

		// Arrow down from last of layouts exits nested context to default
		// appender of root level.
		await page.keyboard.press( 'ArrowDown' );
		await page.keyboard.type( '2nd paragraph' );

		// Arrow up into nested context focuses last text input
		await page.keyboard.press( 'ArrowUp' );
		activeElementText = await page.evaluate( () => document.activeElement.textContent );
		expect( activeElementText ).toBe( '2nd column paragraph' );

		// Arrow up in inner blocks should navigate through (1) column wrapper,
		// (2) text fields.
		// We need to arrow up key presses in the paragraph block because it shows up in two lines.
		await page.keyboard.press( 'ArrowUp' );
		await page.keyboard.press( 'ArrowUp' );
		await page.keyboard.press( 'ArrowUp' );
		activeElementText = await page.evaluate( () => document.activeElement.textContent );
		expect( activeElementText ).toBe( '1st column paragraph' );

		// Arrow up from first text field in nested context focuses column and
		// columns wrappers before escaping out.
		let activeElementBlockType;
		await page.keyboard.press( 'ArrowUp' );
		await page.keyboard.press( 'ArrowUp' );
		activeElementBlockType = await page.evaluate( () => (
			document.activeElement.getAttribute( 'data-type' )
		) );
		expect( activeElementBlockType ).toBe( 'core/column' );
		await page.keyboard.press( 'ArrowUp' );
		activeElementBlockType = await page.evaluate( () => (
			document.activeElement.getAttribute( 'data-type' )
		) );
		expect( activeElementBlockType ).toBe( 'core/columns' );

		// Arrow up from focused (columns) block wrapper exits nested context
		// to prior text input.
		await page.keyboard.press( 'ArrowUp' );
		activeElementText = await page.evaluate( () => document.activeElement.textContent );
		expect( activeElementText ).toBe( '1st paragraph' );

		expect( await getEditedPostContent() ).toMatchSnapshot();
	} );

Because "2nd paragraph" is most likely to wrap on two lines instead of 3.

Look:

I have no idea how to proceed, other than to change the test.

Help me @gziolo test master, you're my only hope!

[gziolo]

@jasmussen - I think we had the same issue in the past but with the different columns setup, let me check how was it fixed and who knows it best :)

[gziolo]

1d5fa32#diff-5ae26f128f656bf9ba7c25b39cc21b8c

@youknowriad is our friend, you can play with the number of ArrowUp key presses like he did in here 1d5fa32#diff-5ae26f128f656bf9ba7c25b39cc21b8c.

Yes, feel free to tweak the test. It seems like a valid requirement to make it pass :)

[youknowriad]

I still want to understand why the column block is showing in three lines now with this PR before updating the test.

[youknowriad]

So it looks like the test is passing for me locally but not in travis which is weird. At this point, I'm in favor of making adjustements to the test to make it pass regardless of the width of the viewport.

[jasmussen]

If I update the snapshots, another test fails:

 FAIL  test/e2e/specs/block-hierarchy-navigation.test.js (6.902s)
  ● Navigating the block hierarchy › should navigate block hierarchy using only the keyboard

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "Navigating the block hierarchy should navigate block hierarchy using only the keyboard 1".

    - Snapshot
    + Received

    @@ -9,9 +9,9 @@
      <div class="wp-block-column"></div>
      <!-- /wp:column -->

      <!-- wp:column -->
      <div class="wp-block-column"><!-- wp:paragraph -->
    - <p>Third column</p>
    + <p></p>
      <!-- /wp:paragraph --></div>
      <!-- /wp:column --></div

I'm not sure I'm doing this right, so I'd appreciate help here.

[jasmussen]

💖💖

[latheeshvm]

is this solved? I am also having this problem..scrolling is not proper with the mouse. it seems to be the scrollbar height is creating the problem, inner one stops at a certain position and outer one need to move up

[aduth]

@latheeshvmv While merged, this is not yet included in a Gutenberg plugin or WordPress release, and is slated to be included in the next minor version of each.

[zignorp]

I just got this as a complaint from a user.

"it seems like as soon as I scroll down to the bottom of the page it prevents me from scrolling back up to the top. This means every time I scroll to the bottom I have to save a draft and start over."

She was also having problems with the classic editor admin, so we switched to gutenberg. Scrolling of the admin is ok for me, but I've got a big window. When I make my window shorter, the scrollbar disappears. I can still scroll using the mouse, but this is difficult for some users with shorter windows. She says she can't click above the nonexistent scrollbar.

[aduth]

The changes here appear to have broken the behavior to click below the blocks region to select the last block / insert a paragraph, in full screen mode.