Closed Bug 1249556 Opened 4 years ago Closed 4 years ago

Support toggle <details> and <summary> by keyboard

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #634004 +++

To break the work down, this bug is for implement the layout part to support toggle <details> and <summary>. The a11y part should be implemented in bug 634004.

I'll add tests and address comments in bug 634004 comment 25.
Component: Disability Access APIs → Layout
The user can switch to the main <summary> by tab key, and toggle the
<details> by either 'spacebar' key or 'enter' key.

Review commit: https://reviewboard.mozilla.org/r/35827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35827/
Attachment #8722007 - Flags: review?(bzbarsky)
Re bug 634004 comment #25:

> This needs tests.
Added in latest patch set.

> >+  const bool defaultFocusable = !aWithMouse;
> 
> This is not using the pref that <input> does, right?  Please document why not.
We should use nsFocusManager::sMouseFocusesFormControl. Fix in latest patch set.

> 
> Should non-main-summary focusability be special too?  If so, please document
> why; I'm not sure it should.
You're right. Focusability of non-main-summary should not be special. Fix in latest patch set.
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

https://reviewboard.mozilla.org/r/35827/#review32551

::: dom/html/HTMLSummaryElement.cpp:62
(Diff revision 1)
> -  // Todo: Bug 634004: Implement toggle details by keyboard.
> +    toggleDetails = ((message == eKeyDown) && (isEnter || isSpace));

Don't need the parens around "message == eKeyDown".

Also, are you sure this should be keydown, not keypress?  That's a change from the previous patch but I'm not clear on why.  

I just tested and at least on Mac Chrome and Safari seem to trigger this on keypress for enter and on keyup (????) for space.  Or something...

::: dom/html/HTMLSummaryElement.cpp:87
(Diff revision 1)
> +  bool notAllowOverrideIsFocusable =

Do you mind adding some documentation in nsGenericHTMLElement.h explaining the semantics of the return value of this method?

::: dom/html/HTMLSummaryElement.cpp:91
(Diff revision 1)
> +    return true;

This seems wrong.  In particular, if !notAllowOverrideIsFocusable but also !IsMainSummary(), I would expect us to return false (like any random HTML element).  So this return statement should probably be:

  return notAllowOverrideIsFocusable;

::: dom/html/HTMLSummaryElement.cpp:103
(Diff revision 1)
> +  return true;

Shouldn't this be return false, since we don't really want to override subclass decisions here?  Or do we?  Eitehr way deserves a comment...

::: dom/html/HTMLSummaryElement.cpp:110
(Diff revision 1)
> +  return IsMainSummary() ? 0 : -1;

I'd prefer we call up to the superclass if !IsMainSummary().
Attachment #8722007 - Flags: review?(bzbarsky)
Just wonder: what will happen when the summary got the contenteditable attribute?
I think there are four possibilities:
- write space (ignore toggle)
- toggle (ignore contenteditable)
- write space and toggle
- nothing

This was already mentioned in bug 591737.
https://reviewboard.mozilla.org/r/35827/#review32551

> Don't need the parens around "message == eKeyDown".
> 
> Also, are you sure this should be keydown, not keypress?  That's a change from the previous patch but I'm not clear on why.  
> 
> I just tested and at least on Mac Chrome and Safari seem to trigger this on keypress for enter and on keyup (????) for space.  Or something...

By peeking Chrome's implementation, your test seems matched the code. 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/HTMLSummaryElement.cpp&sq=package:chromium&type=cs&l=103

I am not sure why Chrome designed to toggle details on keyup for space. I press space on a overflow scroll summary on Chrome, and the summary does not scrolled. Any if we decide to use space and enter to toggle the details, it should not do other things to surprise the user.

Sorry for not being clear on chaning the event type to keydown. I look the DOM level 3 spec, and found that keypress is deprecated. So instead of using keypress with keyCode and charCode, I use keydown with key and code here. This should make no difference when user press the space or enter key, but will affect the event dispatch by script. I'll write a comment about this.
https://www.w3.org/TR/DOM-Level-3-Events/#event-type-keypress

> Do you mind adding some documentation in nsGenericHTMLElement.h explaining the semantics of the return value of this method?

We already have the documentation :)
https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/dom/html/nsGenericHTMLElement.h#596-602

> This seems wrong.  In particular, if !notAllowOverrideIsFocusable but also !IsMainSummary(), I would expect us to return false (like any random HTML element).  So this return statement should probably be:
> 
>   return notAllowOverrideIsFocusable;

My bad...

> Shouldn't this be return false, since we don't really want to override subclass decisions here?  Or do we?  Eitehr way deserves a comment...

HTMLSummaryElement is unlikely to have a subclass. The only special case that we want to override aIsFocusable is mouse clicking on a main summary on Mac like other form elements. It does not harm to give subclass a change to override. I'll add comments.

BTW, I notice that the main summary TabIndexDefault() is 0, which will make it focusable in nsGenericHTMLElement::IsHTMLFocusable(). So the #ifdef block could be simplified.

> I'd prefer we call up to the superclass if !IsMainSummary().

Agree! This is better.
(In reply to sjw from comment #4)
> Just wonder: what will happen when the summary got the contenteditable
> attribute?
> I think there are four possibilities:
> - write space (ignore toggle)
> - toggle (ignore contenteditable)
> - write space and toggle
> - nothing
> 
> This was already mentioned in bug 591737.

I handle keydown on enter and space key and consume the event, so it'll toggle (ignore contenteditable). The user cannot enter space or enter on an editable summary. Both Chrome and Safari have the same behavior.
Rename 'override' to 'notAllowOverrideIsFocusable' in
nsGenericHTMLElement::IsHTMLFocusable() to make the implementation
reflect the semantic that the subclass is not allowed to override the
value returned in aIsFocusable described in nsGenericHTMLElement.h.

Also 'override' is a new specifier since C++11. Rename it make the
syntax highlight looks right.

Review commit: https://reviewboard.mozilla.org/r/36017/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36017/
Attachment #8722417 - Flags: review?(bzbarsky)
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35827/diff/1-2/
Attachment #8722007 - Flags: review?(bzbarsky)
> I am not sure why Chrome designed to toggle details on keyup for space.

Want to ask them?

> I press space on a overflow scroll summary on Chrome, and the summary does not scrolled.

Mmm.  What _should_ happen in this case?  Toggling and scrolling seem somewhat mutually exclusive behaviors here.  What does your impl in Gecko do?

> Any if we decide to use space and enter to toggle the details, it should not do
> other things to surprise the user.

Agreed.

> and found that keypress is deprecated.

First off, the relevant spec is probably https://w3c.github.io/uievents/ not whtever you were reading.

Second, keypress is deprecated for purposes of actual text input, in favor of input events.  But we're not talking about text input here.

The really right thing to do here is probably to fire a DOMActivate off of clicks and whatever key stuff we want, possibly.  Olli, thoughts?

> This should make no difference when user press the space or enter key

It totally does.  Try to press and hold down the enter key in Chrome and with your impl to see the difference.

Also, taking action on keydown is _really_ weird, generally.

> We already have the documentation :)

Ah, right.  I had somehow ended up at https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/dom/html/nsGenericHTMLElement.h#1366 which is a subclass...

> HTMLSummaryElement is unlikely to have a subclass.

Sure.  

> BTW, I notice that the main summary TabIndexDefault() is 0, which will
> make it focusable in nsGenericHTMLElement::IsHTMLFocusable().

That's a bit of action at a distance that I'm wary of depending on.  May be better to have some redundant but clearer code instead.

Anyway, I'd like to figure out what the actual event model we want here is before I start reviewing whether we implement it correctly.
Flags: needinfo?(bugs)
We should get rid of DOMActivate, but if it helps implementation, using the same setup what 
HTMLInputElement has, keeping DOMActivate is fine.
Hmm, and looks like blink is using DOMActivate too here, so perhaps safest to keep using it.

checkboxes and radiobuttons have this historical odd behavior: 'return' is handled with keypress, space with 'keyup'. space + keyup dispatches click event which then toggles the state of checkbox.
But only if preventDefault wasn't called. Following the space + keyup handling here would probably make sense.

I don't know what setActive does in blink. or active(). But I guess the idea is something that
keydown marks the element somehow active, and if still in keyup active, dispatch click, which then dispatches DOMActivate which then toggles (assuming preventDefault() wasn't called in any step). That is close to checkbox handling, and for consistency would make sense.
Flags: needinfo?(bugs)
> checkboxes and radiobuttons have this historical odd behavior: 'return' is handled
> with keypress, space with 'keyup'. 

Right, that's what Chrome seems to implement for <summary> too.  I was thinking about this a bit more, and was going to say that a priori summary and checkbox are very similar beasts in terms of user-perceived interaction (both have an on/off state)...  Looks like that's what you're saying too, in the last part of your comment.

> I don't know what setActive does in blink. or active().

It's used for matching the :active CSS pseudo-class but also for various other stuff like this keyup handling here...  I believe the CSS use is primary and the other stuff is piggybacking, but I could be wrong.
Let me try to summarize what the behavior of summary should be expected.

'return'
- Handle with 'keypress', and consume the event 
- If holding 'enter', we will keep toggling details on and off like Chrome did.

'space'
- On 'keydown', set some internal flag to mark summary 'active'
- Handle with 'keyup' if it's active. Consume the event.
- Consume 'keypress' so that the summary does not scroll.

In both 'return' and 'space', dispatch click event (using DOMActivate?) to toggle the summary. I'll need to study how checkbox works.

Please correct me if I miss something.
DOMActivate is a an event which is dispatched as a default handling for click.
And then DOMActivate's default handling is to do something, like toggle the open state in this case.
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

https://reviewboard.mozilla.org/r/35827/#review33287

Per discussion in bug, this is not what we want.
Attachment #8722007 - Flags: review?(bzbarsky)
Comment on attachment 8722417 [details]
MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz

https://reviewboard.mozilla.org/r/36017/#review33289

::: dom/html/nsGenericHTMLElement.cpp:2707
(Diff revision 1)
> +  bool notAllowOverrideIsFocusable = true;

How about "disallowOverridingFocusability"?

::: dom/html/nsGenericHTMLElement.cpp:2726
(Diff revision 1)
> -    override = true;
> +    notAllowOverrideIsFocusable = true;

It's already true.

r=me
Attachment #8722417 - Flags: review?(bzbarsky)
Attachment #8722417 - Attachment description: MozReview Request: Bug 1249556 - Rename override to notAllowOverrideIsFocusable. r=bz → MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r?bz
Attachment #8722417 - Flags: review?(bzbarsky)
Comment on attachment 8722417 [details]
MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36017/diff/1-2/
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35827/diff/2-3/
Attachment #8722007 - Attachment description: MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=bz → MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r?bz
Attachment #8722007 - Flags: review?(bzbarsky)
https://reviewboard.mozilla.org/r/36017/#review37319

::: dom/html/nsGenericHTMLElement.cpp:2707
(Diff revision 1)
>  bool
>  nsGenericHTMLElement::IsHTMLFocusable(bool aWithMouse,
>                                        bool *aIsFocusable,
>                                        int32_t *aTabIndex)
>  {
> +  bool notAllowOverrideIsFocusable = true;

Fixed.

::: dom/html/nsGenericHTMLElement.cpp:2726
(Diff revision 1)
> +  bool disabled = false;
>  
> -  bool override, disabled = false;
>    if (IsEditableRoot()) {
>      // Editable roots should always be focusable.
> -    override = true;
> +    notAllowOverrideIsFocusable = true;

Fixed.
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

Olli, do you mind taking a look?  Let me know if you want me to sort through the event bits here, but it might be faster for you to....
Attachment #8722007 - Flags: review?(bzbarsky) → review?(bugs)
Attachment #8722417 - Flags: review?(bzbarsky) → review+
Comment on attachment 8722417 [details]
MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz

https://reviewboard.mozilla.org/r/36017/#review37323

r=me
https://reviewboard.mozilla.org/r/35827/#review37321

I'm sorry to postpone addressing the commment of this bug.

::: dom/html/HTMLSummaryElement.cpp:66
(Diff revision 3)
> +
> +      // Both Chrome Safari do not toggle the 'open' attribute on a
> +      // 'display:none' details element. We follow them by checking whether
> +      // details has a frame or not.
> +      if (details->GetPrimaryFrame()) {
> +        details->ToggleOpen();

I fail to figure out the necessaity to fire a DOMActivate event when we got a eMouseClick event, and toggle the detalis element when receving  DOMActivate. So I toggle the details element directly here.

::: dom/html/nsGenericHTMLElement.h:1033
(Diff revision 3)
>    virtual const nsAttrName* InternalGetExistingAttrNameFromQName(const nsAString& aStr) const override;
>  
>    /**
> +   * Dispatch a simulated mouse click by keyboard to the given element.
> +   */
> +  nsresult DispatchSimulatedClick(nsGenericHTMLElement* aElement,

I saw a lot of code duplication used for dispatching mouse click, so I extract this function.
Component: Layout → DOM: Events
Attachment #8722007 - Flags: review?(bugs)
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

https://reviewboard.mozilla.org/r/35827/#review37391

::: dom/html/HTMLSummaryElement.cpp:57
(Diff revision 3)
> +
>    auto* event = aVisitor.mEvent;
>  
>    if (event->HasMouseEventMessage()) {
>      auto* mouseEvent = event->AsMouseEvent();
> -    toggleDetails = mouseEvent->IsLeftClickEvent();
> +    if (mouseEvent->IsLeftClickEvent()) {

Do we need to check here whether the event is trusted? What do other browsers do here?
Does 
summary.dispatchEvent(new MouseEvent("click")); toggle details in other browsers, or is trusted (== user initiated event) needed?

::: dom/html/HTMLSummaryElement.cpp:58
(Diff revision 3)
>    auto* event = aVisitor.mEvent;
>  
>    if (event->HasMouseEventMessage()) {
>      auto* mouseEvent = event->AsMouseEvent();
> -    toggleDetails = mouseEvent->IsLeftClickEvent();
> +    if (mouseEvent->IsLeftClickEvent()) {
> +      auto* details = GetDetails();

You're going to have hard time to sell me 'auto' usage ;)

I don't know from this code what 'details' point to, so I for example don't know whether one should have a strong pointer to it when calling ToggleOpen().
So, please don't use auto here, but the actual type.

And since ToggleOpen() may dispatch mutation events and what not, the type should be RefPtr<HTMLDetailsElement>

::: dom/html/HTMLSummaryElement.cpp:62
(Diff revision 3)
> -    toggleDetails = mouseEvent->IsLeftClickEvent();
> +    if (mouseEvent->IsLeftClickEvent()) {
> +      auto* details = GetDetails();
> +      MOZ_ASSERT(details,
> +                 "Expected to find details since this is the main summary!");
> +
> +      // Both Chrome Safari do not toggle the 'open' attribute on a

Curious, is there a spec bug for this issue, assuming the spec doesn't define this behavior?

::: dom/html/HTMLSummaryElement.cpp:73
(Diff revision 3)
> +        return NS_OK;
> -  }
> +      }
> +    }
> +  } // event->HasMouseEventMessage()
>  
> -  // Todo: Bug 634004: Implement toggle details by keyboard.
> +  if (event->HasKeyEventMessage()) {

Also here, do other browsers support toggling when using untrusted events? (try with new KeyboardEvent())

::: dom/html/HTMLSummaryElement.cpp:74
(Diff revision 3)
> -  }
> +      }
> +    }
> +  } // event->HasMouseEventMessage()
>  
> -  // Todo: Bug 634004: Implement toggle details by keyboard.
> +  if (event->HasKeyEventMessage()) {
> +    auto* keyboardEvent = event->AsKeyboardEvent();

Please don't use 'auto' in cases it isn't clear what the type is.

::: dom/html/HTMLSummaryElement.cpp:88
(Diff revision 3)
>  
> -  auto* details = GetDetails();
> -  MOZ_ASSERT(details, "Expected to find details since this is the main summary!");
> +        dispatchClick = keyboardEvent->keyCode == nsIDOMKeyEvent::DOM_VK_RETURN;
> +        break;
>  
> -  // When dispatching a synthesized mouse click event to a details with
> -  // 'display: none', both Chrome and Safari do not toggle the 'open' attribute.
> +      case eKeyUp:
> +        dispatchClick = keyboardEvent->keyCode == nsIDOMKeyEvent::DOM_VK_RETURN;

This doesn't look right. Don't we want to dispatch click when we get DOM_VK_SPACE in keyup, not RETURN.
Please test both SPACE and RETURN handling.
Attachment #8722007 - Flags: review-
Comment on attachment 8722417 [details]
MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36017/diff/2-3/
Attachment #8722417 - Attachment description: MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r?bz → MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35827/diff/3-4/
Attachment #8722007 - Attachment description: MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r?bz → MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r?smaug
Attachment #8722007 - Flags: review- → review?(bugs)
https://reviewboard.mozilla.org/r/35827/#review37391

> Do we need to check here whether the event is trusted? What do other browsers do here?
> Does 
> summary.dispatchEvent(new MouseEvent("click")); toggle details in other browsers, or is trusted (== user initiated event) needed?

Both Chrome and Safari do toggle the details element by an untrusted event `new MouseEvent("click")`, so I follow their behavior. I use untrusted mouse click events in all my reftest tests.

> You're going to have hard time to sell me 'auto' usage ;)
> 
> I don't know from this code what 'details' point to, so I for example don't know whether one should have a strong pointer to it when calling ToggleOpen().
> So, please don't use auto here, but the actual type.
> 
> And since ToggleOpen() may dispatch mutation events and what not, the type should be RefPtr<HTMLDetailsElement>

'auto' is temptating, but I'll refrain myself from using it ;)

I've changed the usage of auto to explicit type. BTW, I guess it's fine to use 'auto' in cases like `auto* content = static_cast<nsIContent*>(element)` since the type has declared in the `static_cast`.

> Curious, is there a spec bug for this issue, assuming the spec doesn't define this behavior?

Here is the spec issue. https://github.com/whatwg/html/issues/517

> Also here, do other browsers support toggling when using untrusted events? (try with new KeyboardEvent())

Oddly, both Chrome and Safari does not toggle the details element by an untrusted keyboard event, but they did toggle by an unstructed mouse click.

> Please don't use 'auto' in cases it isn't clear what the type is.

Fixed.

> This doesn't look right. Don't we want to dispatch click when we get DOM_VK_SPACE in keyup, not RETURN.
> Please test both SPACE and RETURN handling.

I must mess up the code by replacing NS_VK_SPACE to nsIDOMKeyEvent::DOM_VK_SPACE before pushing to review... My reftest key-space-single-summary.html should catch this fail.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #25) 
> I've changed the usage of auto to explicit type. BTW, I guess it's fine to
> use 'auto' in cases like `auto* content = static_cast<nsIContent*>(element)`
> since the type has declared in the `static_cast`.
Yes, that is fine. Whenever it is clear what the type is, auto is fine.
I general it has been also accepted to use auto also with data structure iterators.
(Though, that latter case can be a bit worrisome in certain cases, since it hides the ownership model.)

We've had security bugs because of use of auto, and we've had memory leaks because of auto.

I know other reviewers aren't as against auto as I am ;)
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

https://reviewboard.mozilla.org/r/35827/#review37675

::: dom/html/HTMLSummaryElement.cpp:66
(Diff revision 4)
> +                 "Expected to find details since this is the main summary!");
> +
> +      // Both Chrome Safari do not toggle the 'open' attribute on a
> +      // 'display:none' details element. We follow them by checking whether
> +      // details has a frame or not.
> +      if (details->GetPrimaryFrame()) {

I think we need to flush layout here so that we don't accidentally expose lazy frame creation to the web.
So, details->GetPrimaryFrame(Flush_Frames)
Attachment #8722007 - Flags: review?(bugs) → review+
Comment on attachment 8722417 [details]
MozReview Request: Bug 1249556 - Rename override to disallowOverridingFocusability. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36017/diff/3-4/
Attachment #8722007 - Attachment description: MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r?smaug → MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug
Comment on attachment 8722007 [details]
MozReview Request: Bug 1249556 - Implement toggling details by keyboard. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35827/diff/4-5/
https://reviewboard.mozilla.org/r/35827/#review37675

> I think we need to flush layout here so that we don't accidentally expose lazy frame creation to the web.
> So, details->GetPrimaryFrame(Flush_Frames)

Fixed.
https://hg.mozilla.org/mozilla-central/rev/1e26562f4f36
https://hg.mozilla.org/mozilla-central/rev/a2fe8535e63b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.