[webkit-reviews] review denied: [Bug 67642] Add a test for accesskey in regard to iframes. : [Attachment 106414] test for accesskey in regard to iframes

Wed Sep 14 21:06:21 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 67642: Add a test for accesskey in regard to iframes.
https://bugs.webkit.org/show_bug.cgi?id=67642

Attachment 106414: test for accesskey in regard to iframes
https://bugs.webkit.org/attachment.cgi?id=106414&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106414&action=review

> LayoutTests/ChangeLog:4
> +

Nit: there should be no blank lines between the bug title and the bug
description.

> LayoutTests/ChangeLog:11
> +	   To catch any improvement of accesskey behavior in regard to
> +	   iframes, it'd be nice to add a test to verify the current
> +	   behavior.

Nit: It seems like we can put this in two lines instead of three. (Note WebKit
doesn't limit the line length at 80 characters).

> LayoutTests/fast/dom/access-key-iframe-expected.txt:12
> +PASS dispatchedEvent("focus") is ["inputC"]
> +PASS dispatchedEvent("focus") is []
> +PASS dispatchedEvent("focus") is ["inputH"]
> +PASS dispatchedEvent("focus") is []
> +PASS dispatchedEvent("focus") is []
> +PASS successfullyParsed is true

These outputs aren't helpful because I can't tell what shows up inside [] is
reasonable or not since there's no description as to what
dispatchedEvent("focus") returns or what had happend before that function is
called.

> LayoutTests/fast/dom/access-key-iframe.html:5
> +<head>
> +<script src="../js/resources/js-test-pre.js"></script>
> +</head>

It seems like you can just put this script element in the body and eliminate
<head> and </head>.

> LayoutTests/fast/dom/access-key-iframe.html:44
> +function dispatchedEvent(eventType)
> +{
> +    var events = eventRecords[eventType];
> +    if (!events)
> +	   return [];
> +    return events;
> +}
> +
> +function recordEvent(event)
> +{
> +    var eventType = event.type
> +    if (!eventRecords[eventType]) {
> +	   eventRecords[eventType] = []
> +    }
> +    eventRecords[eventType].push(event.target.id);
> +}

Why do we need all of this code when we're only listening to focus event?

It seems like we can just define a global variable like targetsOfFocusEvents
that stores the target nodes of all focus events fired.

> LayoutTests/fast/dom/access-key-iframe.html:76
> +    var doc1 = iframe1.contentDocument;
> +
> +    parent = doc1.body;
> +    parent.appendChild(createDomInDocument(doc1, 'input', {'id': 'inputG',
'accesskey': 'a'}));
> +    parent.appendChild(createDomInDocument(doc1, 'input', {'id': 'inputH',
'accesskey': 'c'}));
> +    parent.appendChild(createDomInDocument(doc1, 'input', {'id': 'inputI',
'accesskey': 'd'}));

Instead of using doc1, can we just use iframe1.contentDocument?  That seems to
clarify the semantics of this code.

> LayoutTests/fast/dom/access-key-iframe.html:90
> +    shouldBe('dispatchedEvent("focus")', '["inputC"]');

You could do
shouldBe('setFocus("inputB");pressAccessKey("a");targetsOfFocusEvents',
'["inputC"]');

This should immediately makes clear what is happening prior to evaluating
targetsOfFocusEvents and gives more context in the expected result.
Furthermore, you should probably explain where inputB, etc... belongs (i.e.
document structure) so that people reading the expected results can easily
understand it.