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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
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.

Attachment 106414: test for accesskey in regard to iframes

------- 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

> 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

> 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

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.

More information about the webkit-reviews mailing list