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

Tweak how accesskey on legend works #3987

Merged
merged 10 commits into from
Sep 6, 2018
Merged

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Sep 3, 2018

If the parent is a fieldset, then accesskey on legend delegates to the first appropriate element of the fieldset (but not a label or legend).

Fixes #3950.

Tests:
web-platform-tests/wpt#12800
web-platform-tests/wpt#12801


/interactive-elements.html ( diff )

@zcorpan zcorpan added accessibility Affects accessibility topic: fieldset needs tests Moving the issue forward requires someone to write tests labels Sep 3, 2018
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Sep 3, 2018
@zcorpan zcorpan removed the needs tests Moving the issue forward requires someone to write tests label Sep 3, 2018
@zcorpan
Copy link
Member Author

zcorpan commented Sep 3, 2018

Actually, Edge/Chrome/Safari don't check if the legend is itself focusable. I'll go with the majority.

@zcorpan zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Sep 3, 2018
@zcorpan
Copy link
Member Author

zcorpan commented Sep 3, 2018

@zcorpan zcorpan removed the do not merge yet Pull request must not be merged per rationale in comment label Sep 3, 2018
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

A quick description of the intended change would help with review.

source Outdated
element but that <span data-x="concept-command">defines a command</span>, itself <span
data-x="concept-command">defines a command</span>.</p>
<p>A <code>legend</code> element that has an <span>assigned access key</span> and is a child of
a <code>fieldset</code> element that has a descendant that element and is neither a
Copy link
Member

Choose a reason for hiding this comment

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

This sentence doesn't work.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 5, 2018

Fixed the grammar, and added a description in the OP.

source Outdated
<p>A <code>legend</code> element that has an <span>assigned access key</span> and is a child of
a <code>fieldset</code> element that has a descendant that is neither a <code>label</code>
element nor a <code>legend</code> element but that <span data-x="concept-command">defines a
command</span>, itself <span data-x="concept-command">defines a command</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

So multiple legend element children would be multiple commands?

Also, should there be a comma before "but"?

Copy link
Member Author

Choose a reason for hiding this comment

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

They would, yes. Works in Safari/Chrome/Firefox/Edge.

source Outdated
a descendant of the <code>legend</code> element and is neither a <code>label</code> nor a
<code>legend</code> element.</p>
<code>legend</code> element that <span data-x="concept-command">defines a command</span> but is
neither a <code>label</code> nor a <code>legend</code> element.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Comma before "but"?

Copy link
Member

Choose a reason for hiding this comment

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

This sentence also doesn't quite seem to work. Is it the first element command that is neither a label or legend element or is it the first element command, but only if it's not a label or legend element?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Sorry, I'm still confused. If @domenic is okay with this though I guess it can be merged...

source Outdated
<p>A <code>legend</code> element that has an <span>assigned access key</span> and is a child of
a <code>fieldset</code> element that has a descendant that is neither a <code>label</code>
element nor a <code>legend</code> element, but that <span data-x="concept-command">defines a
command</span>, itself <span data-x="concept-command">defines a command</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I still struggle with this. Perhaps make this a list:

A legend element defines a command if all of the following are true:

  • It has an assigned access key
  • It's a child of a fieldset element
  • Its parent has at least one descendant element that defines a command and is neither a label nor legend element.

source Outdated
a descendant of the <code>legend</code> element and is neither a <code>label</code> nor a
<code>legend</code> element.</p>
<code>legend</code> element that is neither a <code>label</code> nor a <code>legend</code>
element, that <span data-x="concept-command">defines a command</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this still seems ambiguous to me.

@zcorpan
Copy link
Member Author

zcorpan commented Sep 6, 2018

PTAL

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good modulo 1 nit. Given the many turnarounds (apologies for not being clear enough to avoid that) I'd like @domenic to do a final review in case I'm missing something.

source Outdated
<li><p>It has an <span>assigned access key</span>.</p></li>
<li><p>It is a child of a <code>fieldset</code> element.</p></li>
<li><p>Its parent has a descendant that <span data-x="concept-command">defines a command</span>
but is neither a <code>label</code> element nor a <code>legend</code> element.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

and is neither?

@zcorpan zcorpan requested a review from domenic September 6, 2018 14:21
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.

Sorry to drag this out, but I do think there's some real potential for improvement here, which I hope you're up for.

source Outdated
<li><p>It has an <span>assigned access key</span>.</p></li>
<li><p>It is a child of a <code>fieldset</code> element.</p></li>
<li><p>Its parent has a descendant that <span data-x="concept-command">defines a command</span>
and is neither a <code>label</code> element nor a <code>legend</code> element.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

"and is" -> "that is", if I understand correctly??

source Outdated
<li><p>It is a child of a <code>fieldset</code> element.</p></li>
<li><p>Its parent has a descendant that <span data-x="concept-command">defines a command</span>
and is neither a <code>label</code> element nor a <code>legend</code> element.</p></li>
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

This would really benefit from an example or two

source Outdated
<code>legend</code> element.</p>
facets of the first element in <span>tree order</span> that matches all of the following:</p>

<ul>
Copy link
Member

Choose a reason for hiding this comment

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

The duplication here is unfortunate. I'd suggest modifying the previous

Its parent has a descendant that defines a command that is neither a label element nor a legend element

to

Its parent has a descendant that defines a command and is neither a label element nor a legend element. This descendant, if it exists, is the something something.

Then you can say "are the same as the respective facets of the something something."

@zcorpan
Copy link
Member Author

zcorpan commented Sep 6, 2018

@domenic PTAL

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 with nit, nice turnaround time :D

source Outdated
<code>legend</code> element and is neither a <code>label</code> element nor a <code>legend</code>
element but that <span data-x="concept-command">defines a command</span>, itself <span
data-x="concept-command">defines a command</span>.</p>
<p>A <code>legend</code> element <span data-x="concept-command">defines a command</span> if all of the following are true:</p>
Copy link
Member

Choose a reason for hiding this comment

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

100 chars

@zcorpan zcorpan merged commit aa374be into master Sep 6, 2018
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Sep 7, 2018
@annevk annevk deleted the zcorpan/legend-accesskey branch September 7, 2018 08:23
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 12, 2018
Automatic update from web-platform-testsHTML: test accesskey for legend

See whatwg/html#3987
--

wpt-commits: de955843cde5540bfc63015490630beeb1f19f89
wpt-pr: 12800
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 12, 2018
Automatic update from web-platform-testsHTML: test accesskey for legend

See whatwg/html#3987
--

wpt-commits: de955843cde5540bfc63015490630beeb1f19f89
wpt-pr: 12800
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
Automatic update from web-platform-testsHTML: test accesskey for legend

See whatwg/html#3987
--

wpt-commits: de955843cde5540bfc63015490630beeb1f19f89
wpt-pr: 12800

UltraBlame original commit: 0ede957914c48b05a03cf9f754cbd9dfc4cad057
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
Automatic update from web-platform-testsHTML: test accesskey for legend

See whatwg/html#3987
--

wpt-commits: de955843cde5540bfc63015490630beeb1f19f89
wpt-pr: 12800

UltraBlame original commit: 0ede957914c48b05a03cf9f754cbd9dfc4cad057
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
Automatic update from web-platform-testsHTML: test accesskey for legend

See whatwg/html#3987
--

wpt-commits: de955843cde5540bfc63015490630beeb1f19f89
wpt-pr: 12800

UltraBlame original commit: 0ede957914c48b05a03cf9f754cbd9dfc4cad057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants