Bug 49509 – document.activeElement doesn't point to the focused frame

• Component: DOM
• Status: RESOLVED FIXED

[Ojan Vafai]

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.

[Alexey Proskuryakov]

http://msdn.microsoft.com/en-us/library/ms533065(VS.85).aspx

[Ojan Vafai]

Created attachment 74063 [details]
test case

[Erik Arvidsson]

Created attachment 93699 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8700927

[WebKit Review Bot]

Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8699985

[WebKit Review Bot]

Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8705117

[Early Warning System Bot]

Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8705121

[Erik Arvidsson]

Created attachment 93708 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93699 [details]
Patch

Attachment 93699 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8700943

[Ojan Vafai]

whatwg discussion: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031669.html

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

Should we wait a little for responses on WhatWG?

Searching through archives, there was an earlier discussion: <http://www.mail-archive.com/[e-mail]/msg12973.html>. I didn't actually read past the title, so I don't know how closely it's related.

[Ojan Vafai]

(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/[e-mail]/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.

[Erik Arvidsson]

Created attachment 93732 [details]
Patch

[Ryosuke Niwa]

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.

[Ojan Vafai]

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.

[Ryosuke Niwa]

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.

[Erik Arvidsson]

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.

[Erik Arvidsson]

Created attachment 93806 [details]
Patch with updated ChangeLogs

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ian 'Hixie' Hickson]

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

[Erik Arvidsson]

Committed as http://trac.webkit.org/changeset/86707