New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't focus link in new NUX tooltip #7503

Merged
merged 6 commits into from Jun 27, 2018

Conversation

Projects
None yet
5 participants
@tofumatt
Member

tofumatt commented Jun 22, 2018

Description

Changed the focus from the first element in the popover to the container. This is a good compromise between accessibility and style, plus I think it's a bit more intuitive.

Fix #7452.

How has this been tested?

Tabbed and clicked through elements and container was always focused after creation.

Screenshots

2018-06-22 21 37 56

Types of changes

Bug fix I suppose; @karmatosed considered this undesired behaviour 馃槃

Checklist:

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

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Jun 22, 2018

@tofumatt tofumatt changed the title from fix: Don't focus link in new NUX tooltip (fix #7452) to fix: Don't focus link in new NUX tooltip Jun 22, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 22, 2018

I've no idea why this (seemingly) unrelated snapshot test is failing: https://travis-ci.org/WordPress/gutenberg/jobs/395629001#L967

Not failing for me locally either 馃し鈥嶁檪锔

focusNode( firstTabbable ? firstTabbable : this.contentNode.current );
// Focus the popover panel so further tips are easily navigated to via the
// keyboard.
focusNode( this.contentNode.current );

This comment has been minimized.

@youknowriad

youknowriad Jun 22, 2018

Contributor

I also think it's a good compromise between A11y and UX. These buttons focused on popover open are so ugly and confusing (can't distinguish between what's selected and what's focused sometimes).

But this is a very impactful change, because we use Popover a lot. I think we need to test all the other usage carefully.

Ellipsis menu, Block Settings Menu, Block Switcher, Table Block's toolbar, inserter (also mobile). I just wanted to mention it and I'll try to test it later.

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

After testing this, it might actually be better to add a prop that disables focus and applies it to the tooltip only. In a lot of other places (especially Block Switcher) you click on a button and you really want focus in the new popover. In the block selector, for instance, it seems silly to have to type tab first to start searching.

Now I think it's better we focus by default (existing behaviour, largely seems to work) with an option to disable. I'll add that, should be easy (but I'll do it tomorrow, I'ma go read a book or something now 馃槅)

// back to the popover panel itself.
const firstTabbable = focus.tabbable.find( this.contentNode.current )[ 0 ];
focusNode( firstTabbable ? firstTabbable : this.contentNode.current );
// Focus the popover panel so further tips are easily navigated to via the

This comment has been minimized.

@youknowriad

youknowriad Jun 22, 2018

Contributor

Should we avoid mentioning tips because it's not specific to tips?

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

Oh, yes, good call. I was too narrow in my documentation because it's where I tested it.

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 23, 2018

OK from an a11y perspective 馃憤 When the dialog's content is made of text, it's OK to set focus on the first text element, whether it's a heading or paragraph. For reference, see the ARIA Authoring Practices example: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html open the first modal, then click on "Verify address" to open the second modal.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 23, 2018

So it looks like the Popover already has a focusOnMount prop, which is in use. I think it's confusing to have both a focusOnMount and focusFirstElement prop, but given focusOnMount is already in use and its behaviour is different/required by some things, that's what I have now. 馃槥

There's a new prop focusFirstElement that only Tooltip disables.

Focus the *first tabblable element* inside the `Popover` if `focusOnMount` is enabled. This is the default behaviour; set this flag to `false` to focus the container, requiring the user to manually tab to the first element inside the `Popover`.
Has no effect if `focusOnMount` is set to `false`.

This comment has been minimized.

@noisysocks

noisysocks Jun 25, 2018

Member

Just an idea: maybe we could deprecate focusOnMount and use an enum-like property with three states instead?

<Popover autofocus="firstElement" /> // focuses the first tabbable element on mount
<Popover autofocus="container" /> // focuses the container on mount
<Popover autofocus="none" /> // disables focusing on mount

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

I鈥檓 cool with that actually. What鈥檚 our way to throw errors or warn developers when they invariably type something not in the enum list? Mind if I just throw an error?

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

Actually, I wonder about using autofocus because there's already an <input> prop autofocus in HTML.

馃毑馃彔!

This comment has been minimized.

@noisysocks

noisysocks Jun 25, 2018

Member

Maybe leave the name as is then, i.e. focusOnMount="firstElement". Throwing an error or using console.error() works for me 馃檪

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

Oh, actually, that's nice. That way we can just let focusOnMount without a value mean "firstElement" 馃憤

@tofumatt tofumatt requested a review from noisysocks Jun 25, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 25, 2018

@noisysocks Do you care enough about the prop name to wanna change it? If yes I'll do it, otherwise I'm basically of the opinion either of these are "meh, but rarely used" APIs and I'd rather do the one with less overlap with an existing prop and less admin work (deprecating stuff).

But if you feel yours is better, I'll do it. I'm not trying to be lazy, I just don't care so I'm not good to ask about it 馃槅

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 25, 2018

Do you care enough about the prop name to wanna change it?

I think the enum-style property is a nicer API, but it's not a blocker! Agree that it's a rarely used API and so not important. My only argument for changing the API now is that API changes will become very difficult once Gutenberg is merged.

@noisysocks

Tested locally and it works 馃憤

Up to you if you want to try and simplify the Popover API 馃檪

tofumatt added some commits Jun 26, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 26, 2018

Awesome, I went with the simplification; it's enough of a change that another review would be good, sorry! 馃槃

@tofumatt tofumatt requested a review from noisysocks Jun 26, 2018

@noisysocks

I (still) like this. Left some minor comments.

// Boolean values for focusOnMount deprecated in 3.2.
if ( focusOnMount === true ) {
window.console.warn( '<Popover> component: focusOnMount should be false or a string as of Gutenberg 3.2. See: https://github.com/WordPress/gutenberg/tree/master/components/popover/README.md' );

This comment has been minimized.

@noisysocks

noisysocks Jun 27, 2018

Member

I think it's worth using deprecated() here and adding a note to docs/deprecated.md. Then, in 3.4, we can remove the check entirely.

if ( focusOnMount === true ) {
	deprecated( 'focusOnMount={ true }', {
		version: '3.4',
		alternative: 'focusOnMount="firstElement"',
		plugin: 'Gutenberg',
	} );
}

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

Oh, nice! I am total noob so I didn't know how to deprecate stuff. That's totally slick.

// back to the popover panel itself.
const firstTabbable = focus.tabbable.find( this.contentNode.current )[ 0 ];
focusNode( firstTabbable ? firstTabbable : this.contentNode.current );
if ( focusOnMount === 'firstElement' || focusOnMount === true ) {

This comment has been minimized.

@noisysocks

noisysocks Jun 27, 2018

Member

If we do the above then let's make a note here that this focusOnMount === true check can be removed in 3.4.

@@ -221,7 +221,7 @@ class FormatToolbar extends Component {
<div className="editor-format-toolbar__link-container" style={ { ...focusPosition } }>
<Popover
position="bottom center"
focusOnMount={ !! isAddingLink }
focusOnMount={ !! isAddingLink ? 'firstElement' : false }

This comment has been minimized.

@noisysocks

noisysocks Jun 27, 2018

Member

The !! isn't necessary here since we're just using the result in a conditional.

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

iu

But yeah fair enough.

@tofumatt tofumatt added this to the 3.2 milestone Jun 27, 2018

@tofumatt tofumatt merged commit 188b679 into master Jun 27, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +17.48% compared to b16a8b2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the fix/7452-tips-focus-state branch Jun 27, 2018

@@ -271,7 +293,7 @@ class Popover extends Component {
// Apply focus return behavior except when default focus on open
// behavior is disabled.
if ( false !== focusOnMount ) {
if ( ! focusOnMount ) {
content = <FocusManaged>{ content }</FocusManaged>;
}

This comment has been minimized.

@afercia

afercia Jun 27, 2018

Contributor

I'm not sure I understand this change. Unless I'm missing something, the "focus return" behavior should be managed if focusOnMount is truthy. /Cc @tofumatt @noisysocks

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

Oh shoot, I think I had a brain fail. It sucks that tests didn't catch this but I read this as false === focusOnMount not false !== focusOnMount鈥 because the latter seems a weird way to write it.

Follow-up/fix PR incoming, my bad.

This comment has been minimized.

@afercia

afercia Jun 27, 2018

Contributor

I think the previous syntax allowed the prop to be omitted (null) and still have focus managed by default?

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

It has a default prop now; omitting it doesn't set it to null, it sets it to "firstElement":

https://github.com/WordPress/gutenberg/blob/master/components/popover/index.js#L315

This comment has been minimized.

@afercia

afercia Jun 27, 2018

Contributor

Yeah, sorry for cross-commenting on the new PR 馃槈

tofumatt added a commit that referenced this pull request Jun 27, 2018

tofumatt added a commit that referenced this pull request Jun 27, 2018

tofumatt added a commit that referenced this pull request Jun 27, 2018

tofumatt added a commit that referenced this pull request Jun 27, 2018

@aduth aduth referenced this pull request Jul 2, 2018

Merged

Modal component #6261

4 of 4 tasks complete
return;
}
window.console.warn( `<Popover> component: focusOnMount argument "${ focusOnMount }" not recognized.` );

This comment has been minimized.

@aduth

aduth Jul 30, 2018

Member
  • We probably don't need to be logging this (or, by virtue of, including this code at all) in production environments.
  • This is needlessly bound to a window context, when console is more widely available in other environments (Node, React Native)
  • This verges on props typechecking, though I'm not inclined to bring in prop-types at this point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment