Bug 49509 - document.activeElement doesn't point to the focused frame
Summary: document.activeElement doesn't point to the focused frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords: EasyFix
Depends on:
Blocks:
 
Reported: 2010-11-14 07:38 PST by Ojan Vafai
Modified: 2011-05-17 15:06 PDT (History)
11 users (show)

See Also:


Attachments
test case (856 bytes, text/html)
2010-11-16 16:54 PST, Ojan Vafai
no flags Details
Patch (7.45 KB, patch)
2011-05-16 15:03 PDT, Erik Arvidsson
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (7.45 KB, patch)
2011-05-16 15:46 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (8.34 KB, patch)
2011-05-16 19:45 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch with updated ChangeLogs (8.63 KB, patch)
2011-05-17 12:22 PDT, Erik Arvidsson
rniwa: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.22 MB, application/zip)
2011-05-17 12:39 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-11-14 07:38:32 PST
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.
Comment 1 Alexey Proskuryakov 2010-11-15 10:03:43 PST
http://msdn.microsoft.com/en-us/library/ms533065(VS.85).aspx
Comment 2 Ojan Vafai 2010-11-16 16:54:57 PST
Created attachment 74063 [details]
test case
Comment 3 Erik Arvidsson 2011-05-16 15:03:24 PDT
Created attachment 93699 [details]
Patch
Comment 4 Collabora GTK+ EWS bot 2011-05-16 15:09:49 PDT
Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8700927
Comment 5 WebKit Review Bot 2011-05-16 15:11:24 PDT
Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8699985
Comment 6 WebKit Review Bot 2011-05-16 15:11:29 PDT
Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8705117
Comment 7 Early Warning System Bot 2011-05-16 15:13:35 PDT
Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8705121
Comment 8 Erik Arvidsson 2011-05-16 15:46:51 PDT
Created attachment 93708 [details]
Patch
Comment 9 WebKit Review Bot 2011-05-16 16:01:28 PDT
Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8700943
Comment 10 Ojan Vafai 2011-05-16 16:45:23 PDT
whatwg discussion: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031669.html
Comment 11 Ryosuke Niwa 2011-05-16 16:50:32 PDT
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.
Comment 12 Alexey Proskuryakov 2011-05-16 17:29:03 PDT
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.
Comment 13 Ojan Vafai 2011-05-16 18:04:14 PDT
(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.
Comment 14 Erik Arvidsson 2011-05-16 19:45:26 PDT
Created attachment 93732 [details]
Patch
Comment 15 Ryosuke Niwa 2011-05-17 11:59:43 PDT
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 16 Ojan Vafai 2011-05-17 12:15:22 PDT
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 17 Ryosuke Niwa 2011-05-17 12:18:18 PDT
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 18 Erik Arvidsson 2011-05-17 12:20:26 PDT
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.
Comment 19 Erik Arvidsson 2011-05-17 12:22:20 PDT
Created attachment 93806 [details]
Patch with updated ChangeLogs
Comment 20 WebKit Review Bot 2011-05-17 12:39:43 PDT
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
Comment 21 WebKit Review Bot 2011-05-17 12:39:49 PDT
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
Comment 22 Ian 'Hixie' Hickson 2011-05-17 14:52:26 PDT
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.)
Comment 23 Erik Arvidsson 2011-05-17 15:06:26 PDT
Committed as http://trac.webkit.org/changeset/86707