[webkit-reviews] review denied: [Bug 75478] <summary> is not keyboard accessible : [Attachment 126029] Updated patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Arko Saha <nghq36 at motorola.com>'s
request for review:
Bug 75478: <summary> is not keyboard accessible
https://bugs.webkit.org/show_bug.cgi?id=75478

Attachment 126029: Updated patch
https://bugs.webkit.org/attachment.cgi?id=126029&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
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.