Bug 67642

Summary: Add a test for accesskey in regard to iframes.
Product: WebKit Reporter: Hayato Ito <hayato>
Component: DOMAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, dglazkov, dominicc, morrita, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test for accesskey in regard to iframes
none
update1 none

Hayato Ito
Reported 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.
Attachments
test for accesskey in regard to iframes (5.55 KB, patch)
2011-09-06 07:08 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
update1 (6.73 KB, patch)
2011-09-15 05:00 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hayato Ito
Comment 1 2011-09-06 07:08:11 PDT
Created attachment 106414 [details] test for accesskey in regard to iframes
Ryosuke Niwa
Comment 2 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.
Hayato Ito
Comment 3 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.
Hayato Ito
Comment 4 2011-09-15 05:00:07 PDT
Created attachment 107477 [details] update1
WebKit Review Bot
Comment 5 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>
WebKit Review Bot
Comment 6 2011-09-28 20:56:51 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.