Bug 75478 – <summary> is not keyboard accessible

• Component: Accessibility
• Status: RESOLVED FIXED

[Wesley Murch]

It's impossible to TAB into the <details>/<summary> element. It doesn't seem to receive focus, so it requires a mouse click to reveal the <summary>.

Users navigating only by keyboard are unable to view the <summary> contents.

Test case: http://jsfiddle.net/rkq3M/

Related: http://stackoverflow.com/questions/8715462/how-can-i-make-a-details-element-keyboard-accessible

[Wesley Murch]

Mistake: The <summary> content is always visible, the adjacent content is what's inaccessible.

[Alexey Proskuryakov]

I can reproduce this in Safari on Mac, too. Summary isn't focusable even with Option-Tab (or when "Press Tab to highlight each item on a webpage" preference is enabled).

Regular Tab with default preferences shouldn't focus summary, at least on Mac.

[Wesley Murch]

Adding a tabindex does allow the <summary> to have focus, but still seems to do nothing to help show the hidden content (pressing Enter has no effect).

Test case:  http://jsfiddle.net/rkq3M/2/

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=108938

[Arko Saha]

Created attachment 125322 [details]
Proposed patch

This patch contains following:
Show the hidden content of <details> element on pressing Enter key on a focused <details> or <summary> element.
In the specification http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-details-element I couldn't find information related to the activation behavior for <detail> element with Enter key. Here is a proposed patch that expands the <detail> element on pressing the Enter key.
Please let me know your thoughts on the same. Thanks.

[Hajime Morrita]

Comment on attachment 125322 [details]
Proposed patch

Hi Arko, Thanks for taking this!

Here is my thought:
- Without focus, it's not so easy to send a key events to the specific element.
   So it would be great if we can make <summary> focusable.
   There are some elements which support gaining focus and it would be good starting point to follow their way.
   I guess we can handle DOMActivate event.
- On testing, please use EventSender instead of dispatchEvent(). Each of these two goes different code path, 
   and EventSender is suited for emulating user behavior.

[Hajime Morrita]

>    I guess we can handle DOMActivate event.
WebKit fires DOMActivate event only for mouse navigation.
So your approach is basically right. we need to take care of it by ourselves.

[Radar WebKit Bug Importer]

<rdar://problem/10811367>

[Jon Lee]

Comment on attachment 125322 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125322&action=review

> Source/WebCore/html/HTMLSummaryElement.cpp:114
> +static bool isEnterKeydownEvent(Event* event)

Is there a way we can generalize the name of this check so that individual implementations can override? For example, on the Mac platform, opening up a expandable item in an outline view (like a folder in Finder) is possible using the right-arrow key, instead of the enter key. One caveat, though, is that in this case the right-arrow key only opens; it cannot be repeatedly pressed to toggle (you collapse the item by using the left-arrow key).

[Arko Saha]

Hi Morita, Thanks for the review and guidance. Please find my comments below:

(In reply to comment #6)
> Here is my thought:
> - Without focus, it's not so easy to send a key events to the specific element.
>    So it would be great if we can make <summary> focusable.
>    There are some elements which support gaining focus and it would be good starting point to follow their way.

Yes. You are right. We can make <summary> element focusable. But I am not sure whether we should make <summary> or <details> as focusable. As per the spec http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-details-element <details> element is interactive content. So, here I am little confused. Please let me know if I am missing something.

>    I guess we can handle DOMActivate event.
> - On testing, please use EventSender instead of dispatchEvent(). Each of these two goes different code path, 
>    and EventSender is suited for emulating user behavior.

Ok, I will use EventSender in the test-case.

[Arko Saha]

(In reply to comment #9)
> (From update of attachment 125322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125322&action=review
> 
> > Source/WebCore/html/HTMLSummaryElement.cpp:114
> > +static bool isEnterKeydownEvent(Event* event)
> 
> Is there a way we can generalize the name of this check so that individual implementations can override? For example, on the Mac platform, opening up a expandable item in an outline view (like a folder in Finder) is possible using the right-arrow key, instead of the enter key. One caveat, though, is that in this case the right-arrow key only opens; it cannot be repeatedly pressed to toggle (you collapse the item by using the left-arrow key).

Ok, shouldToggle/isActivationKeyPressed/showHideKeyPressed/? any suggestions?

[Ian 'Hixie' Hickson]

The <details> element being defined as "interactive content" doesn't mean anything about how it acts in the UI, it's just a flag for the content model definitions to make it non-conforming for authors to put <details> elements inside <a> or <button> elements, basically.

What the spec says about this specifically is:

"The user agent may also make part of a details element's rendering focusable, to enable the element to be opened or closed using keyboard input. However, this is distinct from the details or summary element being focusable."

[Arko Saha]

Created attachment 125794 [details]
Updated patch

Incorporating review comments.

[WebKit Review Bot]

Comment on attachment 125794 [details]
Updated patch

Attachment 125794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11436265

New failing tests:
fast/html/details-add-summary-10-and-click.html
fast/html/details-add-summary-8-and-click.html
fast/html/details-add-summary-1-and-click.html
fast/html/details-add-summary-2-and-click.html
fast/html/details-remove-summary-6-and-click.html
fast/html/details-add-summary-7-and-click.html
fast/html/details-remove-summary-5-and-click.html
fast/html/details-add-summary-5-and-click.html
fast/html/details-remove-summary-1-and-click.html
fast/html/details-add-summary-6-and-click.html
fast/html/details-remove-summary-4-and-click.html
fast/html/details-add-summary-3-and-click.html
fast/html/details-remove-summary-3-and-click.html
fast/html/details-add-summary-4-and-click.html
fast/html/details-remove-summary-2-and-click.html
fast/html/details-add-summary-9-and-click.html

[Arko Saha]

This is because <summary> is now focusable and clicking it draws a focus ring. So either we need to rebase below tests or we need to make it non-mouse focusable. Any thoughts?

(In reply to comment #14)
> (From update of attachment 125794 [details])
> Attachment 125794 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11436265
> 
> New failing tests:
> fast/html/details-add-summary-10-and-click.html
> fast/html/details-add-summary-8-and-click.html
> fast/html/details-add-summary-1-and-click.html
> fast/html/details-add-summary-2-and-click.html
> fast/html/details-remove-summary-6-and-click.html
> fast/html/details-add-summary-7-and-click.html
> fast/html/details-remove-summary-5-and-click.html
> fast/html/details-add-summary-5-and-click.html
> fast/html/details-remove-summary-1-and-click.html
> fast/html/details-add-summary-6-and-click.html
> fast/html/details-remove-summary-4-and-click.html
> fast/html/details-add-summary-3-and-click.html
> fast/html/details-remove-summary-3-and-click.html
> fast/html/details-add-summary-4-and-click.html
> fast/html/details-remove-summary-2-and-click.html
> fast/html/details-add-summary-9-and-click.html

[Mathias Bynens]

(In reply to comment #5)
> This patch contains following:
> Show the hidden content of <details> element on pressing Enter key on a focused <details> or <summary> element.
> In the specification http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-details-element I couldn't find information related to the activation behavior for <detail> element with Enter key. Here is a proposed patch that expands the <detail> element on pressing the Enter key.

Shouldn’t the Space key have the same effect as Enter when <summary> is focused? I’d expect it to, as that’s how it works for most other keyboard-accessible elements.

[Steve Faulkner]

(In reply to comment #10)
> Hi Morita, Thanks for the review and guidance. Please find my comments below:
> 
> (In reply to comment #6)
> > Here is my thought:
> > - Without focus, it's not so easy to send a key events to the specific element.
> >    So it would be great if we can make <summary> focusable.
> >    There are some elements which support gaining focus and it would be good starting point to follow their way.
> 
> Yes. You are right. We can make <summary> element focusable. But I am not sure whether we should make <summary> or <details> as focusable. As per the spec http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-details-element <details> element is interactive content. So, here I am little confused. Please let me know if I am missing something.
> 
> >    I guess we can handle DOMActivate event.
> > - On testing, please use EventSender instead of dispatchEvent(). Each of these two goes different code path, 
> >    and EventSender is suited for emulating user behavior.
> 
> Ok, I will use EventSender in the test-case.

(In reply to comment #10)
> Hi Morita, Thanks for the review and guidance. Please find my comments below:
> 
> (In reply to comment #6)
> > Here is my thought:
> > - Without focus, it's not so easy to send a key events to the specific element.
> >    So it would be great if we can make <summary> focusable.
> >    There are some elements which support gaining focus and it would be good starting point to follow their way.
> 
> Yes. You are right. We can make <summary> element focusable. But I am not sure whether we should make <summary> or <details> as focusable. As per the spec http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#the-details-element <details> element is interactive content. So, here I am little confused. Please let me know if I am missing something.
> 
> >    I guess we can handle DOMActivate event.
> > - On testing, please use EventSender instead of dispatchEvent(). Each of these two goes different code path, 
> >    and EventSender is suited for emulating user behavior.
> 
> Ok, I will use EventSender in the test-case.

Why make both focusable? Recommend making summary only focusable as it will be mapped as the interactive element in accessibility APIs, the details is just a grouping element.

have provided example accessible implementation information:
HTML to Platform Accessibility APIs Implementation Guide
http://dvcs.w3.org/hg/html-api-map/raw-file/tip/Overview.html#examples

[Arko Saha]

Only <summary> is focusable with latest patch.

> Why make both focusable? Recommend making summary only focusable as it will be mapped as the interactive element in accessibility APIs, the details is just a grouping element.
> 
> have provided example accessible implementation information:
> HTML to Platform Accessibility APIs Implementation Guide
> http://dvcs.w3.org/hg/html-api-map/raw-file/tip/Overview.html#examples

[Arko Saha]

I think Enter key is more suitable than space, so making it default. But platforms can have other options to toggle display by just modifying shouldToggle() for related platform.

(In reply to comment #16)
> Shouldn’t the Space key have the same effect as Enter when <summary> is focused? I’d expect it to, as that’s how it works for most other keyboard-accessible elements.

[Mathias Bynens]

(In reply to comment #19)
> I think Enter key is more suitable than space, so making it default. But
> platforms can have other options to toggle display by just modifying
> shouldToggle() for related platform.

It’s not a matter of XOR, but OR :) Both keys should toggle the contents. http://dvcs.w3.org/hg/html-api-map/raw-file/tip/Overview.html#examples-sum says:

> Pressing the spacebar or enter key when the summary element has focus will
> show the details element content if the content is hidden. If the details
> element content is showing and the summary element has focus, pressing the
> spacebar or enter key will hide the details element content.

[Arko Saha]

(In reply to comment #20)
> It’s not a matter of XOR, but OR :) Both keys should toggle the contents. http://dvcs.w3.org/hg/html-api-map/raw-file/tip/Overview.html#examples-sum says:

Ohh, Ok. Thanks for pointing out. Will update the patch.
 
> > Pressing the spacebar or enter key when the summary element has focus will
> > show the details element content if the content is hidden. If the details
> > element content is showing and the summary element has focus, pressing the
> > spacebar or enter key will hide the details element content.

[Steve Faulkner]

(In reply to comment #19)
> I think Enter key is more suitable than space, so making it default. But platforms can have other options to toggle display by just modifying shouldToggle() for related platform.
> 
> (In reply to comment #16)
> > Shouldn’t the Space key have the same effect as Enter when <summary> is focused? I’d expect it to, as that’s how it works for most other keyboard-accessible elements.

For example on Mac OSX when focused on a disclosure triangle (i.e. what the summary element is) VoiceOver announces: "You are currently on a disclosure traingle to expand or collapse the contents of this item, press control-option-space ('control-option' being the VO modifier keys)"

On windows a button (what the summary element will most likely be mapped to)can be activated with either the space or enter keys, but assistive technology such as screen readers typically announce the space key as the command to activate a button.

[Alexey Proskuryakov]

Note that VoiceOver uses

[Alexey Proskuryakov]

> I think Enter key is more suitable than space, so making it default.

Why do you think so? Generally, Enter submits a form, so it's not a safe key to press. 

> VoiceOver announces

I don't think that VoiceOver cares about what keys we check for here - it handles key presses itself, and uses Accessibility API to implement the desired effect.

This does serve like another example of how Space is preferred indeed.

[Arko Saha]

Created attachment 126029 [details]
Updated patch

Incorporated review comments. Now with the updated patch as per the spec http://dvcs.w3.org/hg/html-api-map/raw-file/tip/Overview.html#examples-sum on pressing the spacebar/enter key on a focused <summary> element will toggle <details> element's content display.

[WebKit Review Bot]

Comment on attachment 126029 [details]
Updated patch

Attachment 126029 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11463350

New failing tests:
fast/html/details-add-summary-10-and-click.html
fast/html/details-remove-summary-4-and-click.html
fast/html/details-add-summary-1-and-click.html
fast/html/details-add-summary-2-and-click.html
fast/html/details-remove-summary-6-and-click.html
fast/html/details-add-summary-7-and-click.html
fast/html/details-add-summary-9-and-click.html
fast/html/details-add-summary-5-and-click.html
fast/html/details-add-summary-6-and-click.html
fast/html/details-add-summary-8-and-click.html
fast/html/details-remove-summary-1-and-click.html
fast/html/details-remove-summary-3-and-click.html
fast/html/details-add-summary-4-and-click.html
fast/html/details-remove-summary-2-and-click.html
fast/html/details-add-summary-3-and-click.html
fast/html/details-remove-summary-5-and-click.html

[Arko Saha]

@Morita: Can you please review the patch. Thanks.

[Hajime Morrita]

Comment on attachment 126029 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126029&action=review

Arko, Thank you for updating the patch. It looks better!
I added some comments.

> Source/WebCore/html/HTMLSummaryElement.cpp:128
> +

Could you check BaseButtonInputType.cpp and HTMLInputElement.cpp to align the behavior? 
In my understanding, it 
- handles DOMActivate event instead of click event (I admit that the current implementation is wrong.)
- has different behaviors for each of enter key and space keys.
- uses Node::dispatchSimulatedClick() to convert key events to click, which eventually triggers DOMActivate event.

> LayoutTests/fast/html/details-keyboard-show-hide.html:24
> +

In general, layout tests should follow some specific patterns.
- You should split action (sending a key event) and check the result (if the open property is expected value) separately.
- For the value comparison, you can use shouldBe() family.
- Emits only minimal logs. Ideally the expectation contains a description line and set of PASS lines.

You can follow the style of well-written tests like ValidityState-valueMissing-002.html.
I admit (again!) that not all existing detail related tests does follow such style.
But I hope new tests to have more "usual" style.

[Arko Saha]

Created attachment 126309 [details]
Updated patch

Incorporating review comments. Modified test-cases. Added failed test cases to chromium/text_expectations.txt.

[Hajime Morrita]

Comment on attachment 126309 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126309&action=review

Looks good! Thanks for taking this. 
Please take care of my nitpick before landing this.

> Source/WebCore/html/HTMLSummaryElement.cpp:121
> +            event->setDefaultHandled();

You can return here.

[Arko Saha]

Created attachment 126717 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 126717 [details]
Updated patch

Clearing flags on attachment: 126717

Committed r107548: <http://trac.webkit.org/changeset/107548>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mathias Bynens]

It seems like this patch didn’t account for <details> elements without a <summary>, in which case a label is automatically generated. I filed bug 78499 for that.