Using NavigableContainer to make a TabPanel for Inserter #3281

ephox-mogran · 2017-11-01T05:32:25Z

Description

Exploring #1823

Created some component for navigation and a11y. Have used them in the InserterMenu as a POC.

How Has This Been Tested?

It's a work in progress. I'll write tests as it's stabilising.

Screenshots (jpeg or gifs if applicable):

Types of changes

New components.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

ephox-mogran · 2017-11-01T05:34:55Z

Basically, there are two bits of feedback that I'd like when people have time:

Are you happy with the architecture of TabPanel and NavigableContainer? Does it look generally useful?
Are you happy with the keyboard flow through the Inserter Menu (excluding pressing Left and Right in the search bar which is currently being interfered with by WritingFlow)? Is that how you would like tabs to work?

The navigation for the InserterMenu is as follows:

Focus starts in search bar.
Pressing tab will shift to the Tabbar and focus the highlighted tab.
Pressing arrow keys will change tabs
Pressing tab from the Tabbar will jump to the first block category
Pressing arrow keys inside the block category will navigate inside that category (in a grid)
Pressing tab from the first block category will jump to the second block category is there is one, or search bar if there isn't
Pressing tab will keep moving through the block categories
Pressing tab on the last block category will jump back to the search bar
Pressing shift+tab at any point will go in the opposite direction

While open, tab and shift+tab will not exit the inserter menu until it is closed with Escape.

Note, the use of stopImmediatePropagation is due to a change where WritingFlow is now binding to the document directly. This is causing many keydowns to shift blocks unexpectedly, unless stopImmediatePropagation is used.

jasmussen · 2017-11-01T07:21:44Z

I like this a lot. Seems to work well. But whether it's okay to do this — require arrow keys to switch tabs — I don't know. @afercia if you have time? Thanks.

Visually the tabs now have rounded corners, but okay to not fix in this branch as I need to revisit focus styles separately as part of #3039

youknowriad

I'm having a hard time reviewing this PR, Do you think we could split it into two PRs, one for the tabs components and one for the NavigableContainer.

Also, having a README explaining all the possible props and properties of a "mode" would help the review.

ephox-mogran · 2017-11-01T10:59:19Z

Yeah, I can add some readme details and tests. Sorry, it's probably not in a review ready state yet. I should have realised.

youknowriad · 2017-11-01T11:02:22Z

Don't worry, it's pretty solid already. Just need some clarity ;)

ephox-mogran · 2017-11-13T23:00:21Z

I've noticed a regression. It isn't focusing the disabled blocks properly. I'll investigate.

ephox-mogran · 2017-11-13T23:13:18Z

The original code has this:

do {
 -			nextIndex += increment;
 -			// Return the name of the next block type.
 -			blockType = blockTypes[ nextIndex ];
 -			if ( blockType && ! this.isDisabledBlock( blockType ) ) {
 -				return blockType.name;
 -			}
 -		} while ( blockType );

So it looks like it was designed to skip over disabled options. This is consistent with the fact that the focus outlines don't appear for disabled elements. Is it reasonable to just make all navigation skip disabled elements then?

ephox-mogran · 2017-11-13T23:24:11Z

Seems like the issue is that when focusable was written, it didn't expect buttons to have a tabindex and be disabled. However, giving a button a tabindex of -1 allows a roving tabindex, which is good for accessibility. Therefore, we can't just OR all of those selectors together any more.

EDIT: Alternatively, we can ensure that we never give a tabindex to anything that is disabled.

…ex to anything disabled

ephox-mogran · 2017-11-14T01:33:58Z

OK. I've tried to update it in line with the PR comments, and also support disabled blocks. I noticed that I had the wrong prop when checking if something should be disabled, so I've fixed that too. @youknowriad @mcsf let me know what else needs to be changed.

ephox-mogran · 2017-11-14T01:58:02Z

Another issue: sometimes pressing tab from the search bar doesn't go into the Search blocks. It looks to be deterministic ... probably something about the roving tab index.

EDIT: boolean logic error when trying to preserve a previous roving tabindex / selection that is still valid. Should be fixed now.

youknowriad · 2017-11-14T08:28:34Z

components/tab-panel/index.js

-	}
-}
+const TabButton = ( { tabId, onClick, children, selected, ...rest } ) => {
+	return <button role="tab"


Minor: might want to use parens around the element here:

return ( <element /> );

youknowriad

I think we're in a good shape here. I found it weird to not be able to navigate the whole list of blocks with arrow keys, I need to tab between categories but I'll defer to @afercia for the right approach here.

There's a small eslint error :)

As a user, pressing the wrong group of arrows (e.g. down instead of right or tab) shouldn't take the focus out of the inserter. This is most easily experienced when using VisualEditorSiblingInserter, the inserter sitting in between any two blocks.

mcsf · 2017-11-14T21:46:10Z

Aside from #3281 (comment), the other odd thing that stuck out as a user was Escape not closing the inserter if focus is in the search input.

I don't see it as a blocker, though.

ephox-mogran · 2017-11-14T21:48:20Z

Interesting. Is that a regression? Or just something we need to implement?

mcsf · 2017-11-14T21:51:04Z

Is that a regression? Or just something we need to implement?

I can't remember. Again, not a blocker, but something to be aware of. If you want to fix it here and the fix is trivial, go for it.

ephox-mogran · 2017-11-14T22:32:31Z

editor/components/inserter/group.js

-			{ blockTypes.map( this.renderItem, this ) }
-		</NavigableMenu>;
+		return (
+			<NavigableMenu


Thanks for the cleanup. Eventually, I hope to get to the point where you don't have to do this every time :)

No worries :), it's almost therapeutic. We just need the robots to do more for us (more linting rules + autoformatters).

ephox-mogran · 2017-11-14T22:58:20Z

Aside from #3281 (comment), the other odd thing that stuck out as a user was Escape not closing the inserter if focus is in the search input.

I can't actually replicate this. I'll merge this one, and we can create a new issue if it happens again.

mcsf · 2017-11-14T23:02:19Z

I can't actually replicate this. I'll merge this one, and we can create a new issue if it happens again.

Oh, you know what, this was almost for sure my Vimium browser extension. I've had it intermittently off for a11y testing. Problem ~~solved~~ never existed. :)

aduth · 2017-11-15T20:13:49Z

components/tab-panel/index.js

+			tabs,
+		} = this.props;
+
+		const selectedTab = tabs.find( ( { name } ) => name === selected );


Array#find is not polyfilled and is not supported by IE11. This results in crashes in IE11.

aduth · 2017-11-15T20:16:14Z

editor/components/inserter/group.js

+		if ( ! isEqual( this.props.blockTypes, nextProps.blockTypes ) ) {
+			this.activeBlocks = deriveActiveBlocks( nextProps.blockTypes );
+			// Try and preserve any still valid selected state.
+			const current = this.activeBlocks.find( block => block.name === this.state.current );


Array#find is not polyfilled and is not supported by IE11. This results in crashes in IE11.

Should we provide a runtime warning in dev env?

Per #746 (comment), I think eventually we'll just want to pull in the polyfill, ideally once Babel 7.x lands.

aduth · 2017-11-27T14:32:29Z

components/tab-panel/index.js

+/**
+ * Internal dependencies
+ */
+import { default as withInstanceId } from '../higher-order/with-instance-id';


You don't need { default as ... } syntax here. This can be simplified to:

import withInstanceId from '../higher-order/with-instance-id';

The default export is inferred when assigning outside the curly braces.

aduth · 2017-11-27T14:34:41Z

editor/components/inserter/menu.js

-			>
-				{ blocks.map( ( block ) => this.getBlockItem( block ) ) }
-			</div>
+			<InserterGroup blockTypes={ blockTypesInfo } labelledBy={ labelledBy }


This convention for applying props is quite difficult to read. I'd suggest either keeping it on a single line, to a certain character limit (80-100), then otherwise consistently applying props one-per-line:

<InserterGroup blockTypes={ blockTypesInfo } labelledBy={ labelledBy } bindReferenceNode={ this.bindReferenceNode } selectBlock={ this.selectBlock } />

Also, we lost the showInsertionPoint, hideInsertionPoint props in this refactor. See #3613.

ephox-mogran requested review from mcsf, youknowriad, afercia, aduth and ellatrix November 1, 2017 05:32

ephox-mogran requested a review from jasmussen November 1, 2017 05:35

ephox-mogran mentioned this pull request Nov 1, 2017

Inserter accessibility #1823

Closed

youknowriad reviewed Nov 1, 2017

View reviewed changes

ephox-mogran added 9 commits November 2, 2017 08:25

navigable container with basic README and skipped tests

51f344a

adding more tests for modes

fc71703

grid navigation for irregular grids

fadd1a2

fixing id in skipped test

51d9e79

replacing other NavigableMenu

3722ef3

using NavigableMenu throughout app

bc2ada7

whitespace

d02c973

accessing DOM (but tests are skipped anyway)

42fa2df

readme fixes and a bit of cleanup

378ad44

ephox-mogran mentioned this pull request Nov 2, 2017

General navigable component #3303

Merged

3 tasks

ephox-mogran added 4 commits November 2, 2017 13:29

initial TabPanel

c000663

using TabPanel in inserter

203a45c

adding role=grid

bccc2b8

only talk about results when searching

ef72bc3

ephox-mogran force-pushed the try/tab-component branch from a0b157f to ef72bc3 Compare November 2, 2017 04:41

ephox-mogran changed the base branch from master to try/general-navigable-component November 2, 2017 04:42

ephox-mogran added 2 commits November 2, 2017 15:19

inserter test fixes

3b08fa8

testing TabPanel

1bc5225

ephox-mogran added 3 commits November 14, 2017 08:10

Merge remote-tracking branch 'origin/master' into try/tab-component

81d490f

defaulting is-active and separating dependencies

90e40bd

using a functional component

de08428

ephox-mogran added 2 commits November 14, 2017 11:21

handling disabled block types

1871b9d

Instead of filtering out disabled focusables, just never add a tabind…

bdfc7a2

…ex to anything disabled

boolean logic error

495b0cb

youknowriad reviewed Nov 14, 2017

View reviewed changes

youknowriad approved these changes Nov 14, 2017

View reviewed changes

mcsf added 2 commits November 14, 2017 21:01

Style fixes & tweaks

dddaae0

mcsf approved these changes Nov 14, 2017

View reviewed changes

ephox-mogran commented Nov 14, 2017

View reviewed changes

ephox-mogran merged commit 4312160 into master Nov 14, 2017

ephox-mogran deleted the try/tab-component branch November 14, 2017 22:58

aduth reviewed Nov 15, 2017

View reviewed changes

aduth mentioned this pull request Nov 15, 2017

Components: Use Lodash's find in place of Array#find #3502

Merged

aduth reviewed Nov 27, 2017

View reviewed changes

afercia mentioned this pull request Apr 21, 2018

Unexpected reverse-tab behaviour #1934

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using NavigableContainer to make a TabPanel for Inserter #3281

Using NavigableContainer to make a TabPanel for Inserter #3281

ephox-mogran commented Nov 1, 2017 •

edited

Loading

ephox-mogran commented Nov 1, 2017 •

edited

Loading

jasmussen commented Nov 1, 2017

youknowriad left a comment

ephox-mogran commented Nov 1, 2017

youknowriad commented Nov 1, 2017

ephox-mogran commented Nov 13, 2017

ephox-mogran commented Nov 13, 2017

ephox-mogran commented Nov 13, 2017 •

edited

Loading

ephox-mogran commented Nov 14, 2017

ephox-mogran commented Nov 14, 2017 •

edited

Loading

youknowriad Nov 14, 2017

youknowriad left a comment •

edited

Loading

mcsf commented Nov 14, 2017

ephox-mogran commented Nov 14, 2017

mcsf commented Nov 14, 2017

ephox-mogran Nov 14, 2017

mcsf Nov 14, 2017

ephox-mogran commented Nov 14, 2017

mcsf commented Nov 14, 2017

aduth Nov 15, 2017 •

edited

Loading

aduth Nov 15, 2017

mcsf Nov 15, 2017

aduth Nov 15, 2017

aduth Nov 27, 2017

aduth Nov 27, 2017

aduth Nov 27, 2017

Using NavigableContainer to make a TabPanel for Inserter #3281

Using NavigableContainer to make a TabPanel for Inserter #3281

Conversation

ephox-mogran commented Nov 1, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

ephox-mogran commented Nov 1, 2017 • edited Loading

jasmussen commented Nov 1, 2017

youknowriad left a comment

Choose a reason for hiding this comment

ephox-mogran commented Nov 1, 2017

youknowriad commented Nov 1, 2017

ephox-mogran commented Nov 13, 2017

ephox-mogran commented Nov 13, 2017

ephox-mogran commented Nov 13, 2017 • edited Loading

ephox-mogran commented Nov 14, 2017

ephox-mogran commented Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

youknowriad left a comment • edited Loading

Choose a reason for hiding this comment

mcsf commented Nov 14, 2017

ephox-mogran commented Nov 14, 2017

mcsf commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Nov 14, 2017

mcsf commented Nov 14, 2017

aduth Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran commented Nov 1, 2017 •

edited

Loading

ephox-mogran commented Nov 1, 2017 •

edited

Loading

ephox-mogran commented Nov 13, 2017 •

edited

Loading

ephox-mogran commented Nov 14, 2017 •

edited

Loading

youknowriad left a comment •

edited

Loading

aduth Nov 15, 2017 •

edited

Loading