Sidebar: Avoid focus loss on active tab change by aduth · Pull Request #10917 · WordPress/gutenberg

aduth · 2018-10-22T19:15:03Z

Partially addresses: #8079

This pull request seeks to resolve an issue of focus loss when changing active sidebar tabs for the editor settings sidebar ("Document", "Block").

Currently, this pull request includes all but the fix itself. Notably, included is an end-to-end test case covering the expected behavior, and a global test event handler protecting against unexpected focus loss.

Update: It has been implemented with 72569ab.

Implementation notes:

Focus loss occurs because each specific sidebar renders the entirety of the .edit-post-wrapper element. For editor settings, this also includes the tab header as well.

The bug occurs because React is unable to reconcile the two element trees, and is therefore forced to unmount and remount an entirely new sidebar wrapper.

An initial attempt here was to invert rendering, having each specific sidebar populating only the Fill of the sidebar component, whereas the Slot would be responsible for rendering the .edit-post-wrapper wrapper element. This has the added advantage of fixing an issue with the application of withFocusReturn, as currently this is broken since it's applied to the Fill (non-DOM) element. An issue with this approach is in respecting the aria-label application provided by each specific sidebar.

Testing instructions:

Verify that no focus loss occurs when changing tabs in the sidebar.

Navigate to Posts > Add New
Click "Document"
Press Tab
Press Spacebar
Note that focus remains on the "Block" tab after activation

Ensure end-to-end tests pass:

npm run test-e2e

gziolo · 2018-10-23T06:09:32Z

Thanks @aduth, this is very helpful. I will try to find some time today to start working on a fix.

gziolo · 2018-10-24T11:11:26Z

I consolidate both sidebars into one component with 678819 and it solves the issue. It reuses the same Fill with the updated implementation so it is able to re-render (instead of unmount and mount) even when the active tab changes.

gziolo · 2018-10-24T11:13:20Z

@vindl - just letting you know that this changes a bit the logic inside the edit-post's Layout component.

gziolo · 2018-10-25T05:18:59Z

@afercia this should be ready to verify 👍

E2e tests fail but this is unrelated. I bet focus loss check should be enabled only for tests which use keyboard navigation, not to all of them.

afercia · 2018-10-25T08:19:23Z

@gziolo thanks. I'm not seeing the sidebar at all. When I click the Settings button, I see the is-sidebar-open CSS class gets applied but nothing is rendered, no sidebar. What I'm doing wrong?

gziolo · 2018-10-25T09:58:22Z

@aduth - it seems to fail too many test suites. I moved it to utils and applied only for the sidebar. Let's open a follow-up and this assertion where it fits.

I also reverted changes applied to setup-test-framework.js with 46a078f.

There is this issue that afterEach removes all page listener but afterAll needs them, too.

gziolo · 2018-10-25T10:00:04Z

@afercia it was my mistake, it should be fixed with d5ff172#diff-b006c664acf96e390063ef30c215b267R60 - I did a typo when refactoring code at the end of the day ...

afercia · 2018-10-25T10:17:14Z

@gziolo LGTM 👍 thanks!

tofumatt

Ran the test steps locally and it worked a treat. The code makes sense and the tests passed, so

aduth · 2018-10-25T20:58:28Z

-	>
-		<SettingsHeader sidebarName={ SIDEBAR_NAME } />
-		<Panel>
-			<PanelBody className="edit-post-block-sidebar__panel">


There's still an occurrence of this now-removed class name in tests:

gutenberg/test/e2e/specs/block-hierarchy-navigation.test.js

Line 35 in dd7e54b

await page.focus( '.edit-post-block-sidebar__panel .components-range-control__number[aria-label="Columns"]' );

Tests are currently running, but if they're passing that would be equally concerning.

tofumatt · 2018-10-25T21:03:03Z

Update: I ran the tests on master by mistake but I verified the fix manually on the branch, sorry. Running them on the right branch now 😓

tofumatt

Locally I'm getting a failure on:

>   ● splitting and merging blocks › should remove at most one paragraph in forward direction

Sorry, I thought I clicked "submit review" hours ago but I guess not, I'm just the worst on this PR 😓

From the looks the snapshot is wrong so something is off here and needs fixing.

gziolo · 2018-10-26T07:18:12Z


 		// Tweak the columns count.
-		await page.focus( '.edit-post-block-sidebar__panel .components-range-control__number[aria-label="Columns"]' );
+		await page.focus( '.edit-post-settings-sidebar__panel-block .components-range-control__number[aria-label="Columns"]' );


Good catch, thanks 👍

removePageEvents now called in afterEach

gziolo · 2018-10-26T09:33:54Z

● splitting and merging blocks › should remove at most one paragraph in forward direction

@tofumatt - it works properly for me locally and on Travis

gziolo · 2018-10-26T10:03:48Z

Travis is finally happy! 🎉

aduth · 2018-10-26T12:35:07Z

In spirit I give my approval, though despite @gziolo doing the bulk of the work, I can't review a pull request I'd opened 😄

youknowriad

despite @aduth doing the bulk of review work, he can't review the pull request he opened 😄so I'm approving :P

Test failure resolved #10917 (comment)

aduth assigned gziolo Oct 22, 2018

aduth added the [Status] In Progress Tracking issues with work in progress label Oct 22, 2018

gziolo mentioned this pull request Oct 23, 2018

Sidebar header: avoid focus loss and other improvements #8079

Closed

gziolo removed the [Status] In Progress Tracking issues with work in progress label Oct 24, 2018

gziolo requested a review from tofumatt October 24, 2018 11:08

gziolo added the Needs Accessibility Feedback Need input from accessibility label Oct 24, 2018

gziolo requested a review from vindl October 24, 2018 11:12

gziolo requested a review from a team October 24, 2018 11:13

gziolo added this to the 4.2 milestone Oct 24, 2018

gziolo force-pushed the fix/focus-loss-sidebar-tab branch from 56ab3c8 to d5ff172 Compare October 25, 2018 09:54

gziolo reviewed Oct 25, 2018

View reviewed changes

gziolo force-pushed the fix/focus-loss-sidebar-tab branch from 0004cb7 to dd7e54b Compare October 25, 2018 10:34

tofumatt approved these changes Oct 25, 2018

View reviewed changes

aduth commented Oct 25, 2018

View reviewed changes

tofumatt previously requested changes Oct 25, 2018

View reviewed changes

gziolo reviewed Oct 26, 2018

View reviewed changes

aduth added 3 commits October 26, 2018 10:52

Testing: Drop suffix in sidebar test file name

1cd7dd6

Testing: Guarantee viewport size reset between tests

06c67db

Testing: Observe and throw on focus loss

a1c63b3

aduth and others added 7 commits October 26, 2018 10:52

Testing: Add test for Sidebar tab activation focus

4f50259

Fix focus loss on active tab change in Settings Sidebar

fb6ac37

Reuse isEditorSidebarOpened selector to simplify logic

3e2d535

Apply focus loss check only in the sidebar test

e820440

Apply focus loss check only in the sidebar test

379fe6c

Testing: Update class name for block settings sidebar

8aa9469

Testing: Remove redundant afterAll

f779391

removePageEvents now called in afterEach

gziolo force-pushed the fix/focus-loss-sidebar-tab branch from 4d5beff to f779391 Compare October 26, 2018 08:52

Revert changes introduced to setup-test-framework

46a078f

gziolo requested a review from tofumatt October 26, 2018 10:03

youknowriad approved these changes Oct 26, 2018

View reviewed changes

aduth merged commit e8de101 into master Oct 26, 2018

aduth deleted the fix/focus-loss-sidebar-tab branch October 26, 2018 13:25

Mamaduka mentioned this pull request Dec 29, 2023

Migrate 'sidebar' e2e tests to Playwright #57448

Merged

Conversation

aduth commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Oct 23, 2018

Uh oh!

gziolo commented Oct 24, 2018

Uh oh!

gziolo commented Oct 24, 2018

Uh oh!

gziolo commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afercia commented Oct 25, 2018

Uh oh!

gziolo Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Oct 25, 2018

Uh oh!

afercia commented Oct 25, 2018

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

tofumatt commented Oct 25, 2018

Uh oh!

tofumatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

gziolo commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Oct 26, 2018

Uh oh!

aduth commented Oct 26, 2018

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aduth commented Oct 22, 2018 •

edited

Loading

gziolo commented Oct 25, 2018 •

edited

Loading

gziolo Oct 26, 2018 •

edited

Loading

tofumatt left a comment •

edited

Loading

gziolo commented Oct 26, 2018 •

edited

Loading