Bug 67642 - Add a test for accesskey in regard to iframes.
Summary: Add a test for accesskey in regard to iframes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-06 06:57 PDT by Hayato Ito
Modified: 2011-09-28 20:56 PDT (History)
5 users (show)

See Also:


Attachments
test for accesskey in regard to iframes (5.55 KB, patch)
2011-09-06 07:08 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
update1 (6.73 KB, patch)
2011-09-15 05:00 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-09-06 06:57:16 PDT
This is a derived from the discussion of https://bugs.webkit.org/show_bug.cgi?id=67096.

It might be nice if there was a test that verifies the current accesskey behavior in regard to iframes to catch up if someone improves accesskey for iframes.
Comment 1 Hayato Ito 2011-09-06 07:08:11 PDT
Created attachment 106414 [details]
test for accesskey in regard to iframes
Comment 2 Ryosuke Niwa 2011-09-14 21:06:22 PDT
Comment on attachment 106414 [details]
test for accesskey in regard to iframes

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.
Comment 3 Hayato Ito 2011-09-15 04:59:07 PDT
Comment on attachment 106414 [details]
test for accesskey in regard to iframes

Thank you for the review. I've addressed your comments.

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.

Done.

>> LayoutTests/ChangeLog:11
>> +        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).

Done.

>> LayoutTests/fast/dom/access-key-iframe.html:5
>> +</head>
> 
> It seems like you can just put this script element in the body and eliminate <head> and </head>.

Done.

>> LayoutTests/fast/dom/access-key-iframe.html:44
>> +}
> 
> 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.

Yeah, that's overkill. This function was simply copied and pasted from other test which has to record all kinds of events.

I've simplified that as you suggested. Thank you.

>> LayoutTests/fast/dom/access-key-iframe.html:76
>> +    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.

Sure. Done.

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

That sounds nice.
I've updated the testing code, adding some descriptions.
Comment 4 Hayato Ito 2011-09-15 05:00:07 PDT
Created attachment 107477 [details]
update1
Comment 5 WebKit Review Bot 2011-09-28 20:56:47 PDT
Comment on attachment 107477 [details]
update1

Clearing flags on attachment: 107477

Committed r96292: <http://trac.webkit.org/changeset/96292>
Comment 6 WebKit Review Bot 2011-09-28 20:56:51 PDT
All reviewed patches have been landed.  Closing bug.