Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define different types of "focusable" & remove "tabindex focus flag" #4768

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Define different types of "focusable" & remove "tabindex focus flag" #4768

merged 1 commit into from
Sep 24, 2019

Conversation

rakina
Copy link
Member

@rakina rakina commented Jul 12, 2019

One of the changes suggested from #4607.

This defines "focusable" and also defines special "types" of focusability: "sequentially focusable" and "click focusable". This also removes "tabindex focus flag" because it can be replaced with "focusable"/"focusable area". These changes are made on top of changes from #4735, because of some changes in tabindex etc in that PR.

No normative changes so no WPT.


/canvas.html ( diff )
/custom-elements.html ( diff )
/indices.html ( diff )
/interaction.html ( diff )
/rendering.html ( diff )
/semantics-other.html ( diff )

@domenic
Copy link
Member

domenic commented Jul 12, 2019

Now that I see this written up, I think it makes sense to not have a separate "programatically focusable" term; just "focusable" is good enough. We will want to add an explanation, though, for what it means for something to be "focusable" but neither click nor sequentially focusable---it means that .focus() and .activeElement can work, even if user interaction won't make something focusable. Probably we can give a concrete example as well, at least one that works in most browsers.

Maybe even "focusable" is redundant with "focusable area"?

Anyway, this is a bit hard to review in detail because of how it's based on #4735. We can wait for that to land, or you could make it easier by squashing all new changes into a single commit, so I can review just that one commit.

@domenic domenic added clarification Standard could be clearer topic: focus labels Jul 12, 2019
@rakina
Copy link
Member Author

rakina commented Jul 16, 2019

Now that I see this written up, I think it makes sense to not have a separate "programatically focusable" term; just "focusable" is good enough. We will want to add an explanation, though, for what it means for something to be "focusable" but neither click nor sequentially focusable---it means that .focus() and .activeElement can work, even if user interaction won't make something focusable. Probably we can give a concrete example as well, at least one that works in most browsers.

Maybe even "focusable" is redundant with "focusable area"?

Yeah I agree. Removed "programatically focusable", but kept "focusable" mostly because maybe it's nicer than saying "is a focusable area" everywhere? I'm ok with removing it too though.

Anyway, this is a bit hard to review in detail because of how it's based on #4735. We can wait for that to land, or you could make it easier by squashing all new changes into a single commit, so I can review just that one commit.

Not sure if I did it correctly, but here's all the changes in one commit, I think: 76591a8

@rakina rakina marked this pull request as ready for review July 16, 2019 04:07
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Just some editorial nits. This is so much clearer.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
@@ -73861,8 +73897,8 @@ END:VCARD</pre>

<dd>

<p>The user agent must set the element's <span>tabindex focus flag</span>, but should omit the
element from any <span>tabindex-ordered focus navigation scope</span>.</p>
<p>The user agent must allow the element to be considered as a <span>focusable area</span>, but
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "must consider the element as a"? I think this is stronger than just "allow"...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah I originally made this weak because of the differences between OSes thing, but I changed it to your suggestion just now.

source Outdated
@@ -73935,7 +73971,7 @@ END:VCARD</pre>
</dl>

<p>An element with the <code data-x="attr-tabindex">tabindex</code> attribute specified is
<span>interactive content</span>.</p>
<span>interactive content</span>.<!-- should this check for focusability too ?--></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I think no, because this is about conformance checkers, which will generally just look at the DOM structure, not complicated UA-specific factors. So we can remove this note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks!

@domenic domenic self-assigned this Jul 30, 2019
@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Aug 6, 2019
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Marking "do not merge yet" until #4735 lands.

@domenic
Copy link
Member

domenic commented Aug 29, 2019

After explaining some stuff to several web devs over the last couple of days, I pushed a commit that includes a non-normative summary which should make things clearer. I also made it explicit that having a tabindex="" attribute 'should' cause you to be click focusable, as that's generally true but we weren't saying it before.

@rakina
Copy link
Member Author

rakina commented Aug 30, 2019

After explaining some stuff to several web devs over the last couple of days, I pushed a commit that includes a non-normative summary which should make things clearer. I also made it explicit that having a tabindex="" attribute 'should' cause you to be click focusable, as that's generally true but we weren't saying it before.

Thank you! Definitely useful to have the easier-to-understand summary.

Copy link

@smaug---- smaug---- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't really reviewable since this somehow contains #4735 or parts of it

@rakina
Copy link
Member Author

rakina commented Sep 2, 2019

this isn't really reviewable since this somehow contains #4735 or parts of it

You can select the commits that aren't part of PR 4735 in the "files changed" tab: https://github.com/whatwg/html/pull/4768/files/4e2a998af84570fda3ff90d45a1882408c9f1cbb..75b444334fe8a77d6e26ee8b4c160ea6cebf3d93

Define "focusable" more concretely, and as part of it, define special
types of focusability: click focusable and sequentially focusable.

As part of this, remove the "tabindex focus flag" because it can be
replaced with "focusable" and "focusable area", and was very confusing.

Part of #4607. Helps provide a basis for further work on #2013, but does
not directly contribute to any shadow DOM upstreaming.

This does not introduce any normative changes, but instead brings into
the spec behavior that was previously only in implementations, and makes
certain concepts explicit.
@domenic domenic merged commit 1ab7c86 into whatwg:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: focus
Development

Successfully merging this pull request may close these issues.

3 participants