When a the contents of a frame have focus, document.activeElement in the parent should point to the frame. Right now it points to the body in the parent. Opera matches WebKit behavior. Firefox and IE have activeElement point to the frame.
http://msdn.microsoft.com/en-us/library/ms533065(VS.85).aspx
Created attachment 74063 [details] test case
Created attachment 93699 [details] Patch
Comment on attachment 93699 [details] Patch Attachment 93699 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8700927
Comment on attachment 93699 [details] Patch Attachment 93699 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8699985
Comment on attachment 93699 [details] Patch Attachment 93699 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8705117
Comment on attachment 93699 [details] Patch Attachment 93699 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8705121
Created attachment 93708 [details] Patch
Comment on attachment 93699 [details] Patch Attachment 93699 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8700943
whatwg discussion: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031669.html
Comment on attachment 93708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93708&action=review Looks sane to me but please fix nits. > LayoutTests/fast/dom/HTMLDocument/active-element-frames-expected.txt:9 > +PASS: doc0.activeElement is input0 > +PASS: doc1.activeElement is body1 > +PASS: doc2.activeElement is body2 > +PASS: doc3.activeElement is body3 > +PASS: doc4.activeElement is body4 All these outputs make little sense unless you know the test code. Could you show the structure of the document used in this test? > LayoutTests/fast/dom/HTMLDocument/active-element-frames-expected.txt:11 > +Focusing input1 This output isn't useful. What does input1 mean? You should explain that this is an input element inside a document. > Source/WebCore/html/HTMLDocument.cpp:144 > + } else { I would have added an early exit above the else to avoid adding the else clause. > Source/WebCore/html/HTMLDocument.cpp:146 > + Page* page = this->page(); > + if (page) { You should either declare page inside the if's condition or early exit when !page. > Source/WebCore/html/HTMLDocument.cpp:147 > + for (Frame* f = page->focusController()->focusedFrame(); f; f = f->tree()->parent()) { Please do not use one letter variable.
Should we wait a little for responses on WhatWG? Searching through archives, there was an earlier discussion: <http://www.mail-archive.com/whatwg@lists.whatwg.org/msg12973.html>. I didn't actually read past the title, so I don't know how closely it's related.
(In reply to comment #12) > Should we wait a little for responses on WhatWG? > > Searching through archives, there was an earlier discussion: <http://www.mail-archive.com/whatwg@lists.whatwg.org/msg12973.html>. I didn't actually read past the title, so I don't know how closely it's related. This specific issue isn't addressed in any past discussion I could find. This doesn't seem especially controversial to me though since Firefox and IE already have this behavior. With WebKit matching, it's hard to imagine the spec would not follow.
Created attachment 93732 [details] Patch
Comment on attachment 93732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93732&action=review > LayoutTests/ChangeLog:7 > + You should explain what kind of a test you're adding. > LayoutTests/fast/dom/HTMLDocument/active-element-frames.html:3 > + I don't think we want these blank lines. They clutter the test code. > LayoutTests/fast/dom/HTMLDocument/active-element-frames.html:40 > +var iframe1 = doc0.getElementById('frame-1'); > +var doc1 = iframe1.contentDocument; > +var input1 = doc1.body.appendChild(doc1.createElement('input')); > + > +var iframe2 = doc0.getElementById('frame-2'); > +var doc2 = iframe2.contentDocument; > +var input2 = doc2.body.appendChild(doc2.createElement('input')); > + > +var iframe3 = doc1.body.appendChild(doc1.createElement('iframe')); > +var doc3 = iframe3.contentDocument; > +var input3 = doc3.body.appendChild(doc3.createElement('input')); > + > +var iframe4 = doc2.body.appendChild(doc2.createElement('iframe')); > +var doc4 = iframe4.contentDocument; > +var input4 = doc4.body.appendChild(doc4.createElement('input')); > + > +// Set up IDs for logging. > +for (var i = 0; i < 5; i++) { > + window['doc' + i].body.id = 'body' + i; > + if (i > 0) > + window['iframe' + i].id = 'iframe' + i; > + window['input' + i].id = 'input' + i; > +} It's not obvious which iframe/doc/input gets which id from this code. Could you either hard-code everything or generate everything in the loop? > Source/WebCore/ChangeLog:7 > + You should refer to relevant spec. or justification as to why this behavior is desirable.
Comment on attachment 93732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93732&action=review >> LayoutTests/ChangeLog:7 >> + > > You should explain what kind of a test you're adding. What sort of explanation do you have in mind? It seems pretty self-explanatory to me. >> LayoutTests/fast/dom/HTMLDocument/active-element-frames.html:3 >> + > > I don't think we want these blank lines. They clutter the test code. I don't see what the problem here is. Making the test more readable at the cost of a few extra spaces in the expected.txt output doesn't seem bad to me.
Comment on attachment 93732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93732&action=review >>> LayoutTests/ChangeLog:7 >>> + >> >> You should explain what kind of a test you're adding. > > What sort of explanation do you have in mind? It seems pretty self-explanatory to me. Something like "Added a test to ensure WebKit returns the frame element as the active element when the focus is inside the frame." >>> LayoutTests/fast/dom/HTMLDocument/active-element-frames.html:3 >>> + >> >> I don't think we want these blank lines. They clutter the test code. > > I don't see what the problem here is. Making the test more readable at the cost of a few extra spaces in the expected.txt output doesn't seem bad to me. To me at least, these blank lines make the test less readable.
Comment on attachment 93732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93732&action=review >> LayoutTests/fast/dom/HTMLDocument/active-element-frames.html:40 >> +} > > It's not obvious which iframe/doc/input gets which id from this code. Could you either hard-code everything or generate everything in the loop? It does not matter what ID they get. The ID is only used to distinguish the elements. The describe function is used to give you a meaningful description of where each element is in the DOM.
Created attachment 93806 [details] Patch with updated ChangeLogs
Comment on attachment 93806 [details] Patch with updated ChangeLogs Attachment 93806 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8667025 New failing tests: fast/dom/HTMLDocument/active-element-frames.html
Created attachment 93809 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Do what Firefox and IE do, I'll update the spec accordingly when I get to that feedback. (Sorry, I'm months behind at the moment due to the HTMLWG pre-last-call process.)
Committed as http://trac.webkit.org/changeset/86707