Bug 32292 – Unable to focus on embedded plugins such as Flash via javascript focus()

• Component: Plug-ins
• Status: RESOLVED FIXED

[Dave Myron]

The simplest test to reproduce:

1. Go to a Youtube video.
2. In the location bar, type: javascript:document.getElementById('movie_player').focus();
3. Push the space bar 

Expected behavior (experienced in IE and Firefox):
The video toggles between playing and paused with each press of the space bar.

Observed behavior in Safari and Chrome:
Nothing. It would appear *something* gets focus, however, since the page doesn't scroll as it normally would.

[Paul Dragoonis]

I have been advised by the #webkit guys on IRC to post this comment here for cross-reference:
https://bugzilla.mozilla.org/show_bug.cgi?id=93149

Here is also another bug on the chromium side relating to the same bug.

http://code.google.com/p/chromium/issues/detail?id=27868


I hope this issue gets fixed soon because it makes safari and chrome not an option for accesibility tools to tab in/out of flash elements.

[Alexey Proskuryakov]

HTMLPlugInImageElement probably needs a custom setFocus() implementation, similar to HTMLFrameElementBase.

[Sunil]

I have found that giving the embedded flash object a tabIndex allows me to focus the flash app in Safari but not Chrome. 

However, Safari has other issues. If I recall correctly, I had problems focusing HTML elements after I set focus on flash. 

I am working on accessibility and the very large corporation I work for has decided they won't support webkit browsers because of these issues.

[Dave Michael]

Created attachment 121507 [details]
Patch

[Adam Barth]

Comment on attachment 121507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121507&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:200
> +    if (Page* page = document()->page())
> +        if (received)
> +            page->focusController()->setFocusedNode(this, contentFrame());

This if statement should have { } because it takes up more than one line.

> Source/WebCore/html/HTMLPlugInElement.h:-71
> -private:

Did you mean to remove this declaration?

> Source/WebCore/html/HTMLPlugInElement.h:72
> +    virtual bool supportsFocus() const;
> +    virtual void setFocus(bool);

Should these have the OVERRIDE keyword (at the end)?

[Ryosuke Niwa]

Comment on attachment 121507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121507&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:192
> +    return true;

I don't think we can always return true here e.g. when we have fallback contents displayed.

[WebKit Review Bot]

Comment on attachment 121507 [details]
Patch

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

New failing tests:
fast/events/tabindex-focus-blur-all.html

[Dave Michael]

Created attachment 121904 [details]
Patch

[Dave Michael]

Created attachment 121906 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 121906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121906&action=review

> Source/WebCore/ChangeLog:7
> +

You should explain what kind of changes you're making.

> Source/WebCore/html/HTMLAppletElement.h:46
> +    virtual bool useFallbackContent() const OVERRIDE { return false; }

Instead of making HTMLPluginElement:: useFallbackContent a pure virtual, you can move the definition in HTMLPluginImageElement to HTMLPluginElement so that you don't have to add new definition here.

> Source/WebCore/html/HTMLPlugInElement.cpp:192
> +    if (useFallbackContent())

You should also make sure HTMLFrameOwnerElement::supportsFocus() returns true. e.g. if this plugin is showing a fallback content and has contenteditable content attribute, then this element should be focusible (we need a test case for this). r- because of this.

> Source/WebCore/html/HTMLPlugInElement.cpp:194
> +    RenderObject* r = renderer();

Please don't use one-letter variable. Also, renderer() is an inline function so there's no need to cache it.

> Source/WebCore/html/HTMLPlugInElement.cpp:195
> +    if (!r || !(r->isEmbeddedObject()))

Redundant parenthesis around r->isEmbeddedObject(). This condition should be re-written as:
!renderer() || !renderer()->isEmbeddedObject()

> Source/WebCore/html/HTMLPlugInElement.cpp:197
> +    RenderEmbeddedObject* renderEmbed(toRenderEmbeddedObject(r));

Please use operator= here: renderEmbed = toRenderEmbeddedObject(renderer()).

> Source/WebCore/html/HTMLPlugInElement.cpp:209
> +        if (received)
> +            page->focusController()->setFocusedNode(this, contentFrame());

Could you explain why we need this code? Also, maybe we can share code with HTMLFrameElementBase::setFocus ?

> LayoutTests/ChangeLog:7
> +

Please explain what kind of a test you're adding.

> LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
> -333 focus / 333 blur events dispatched, and should be 333 / 333 PASSED
> +336 focus / 336 blur events dispatched, and should be 336 / 336 PASSED

Why is this change not listed on LayoutTests/ChangeLog ?

> LayoutTests/plugins/focus.html:1
> +<html>

Are you intentionally testing quirks mode? If not, we need <!DOCTYPE html> here.

> LayoutTests/plugins/focus.html:29
> +        var elem = owner.childNodes[i];

Please don't use abbreviations like elem.

> LayoutTests/plugins/focus.html:33
> +            real_target = undefined;

Please use camelCase.

> LayoutTests/plugins/focus.html:41
> +            if (elem.getAttribute("should_focus") === "true") {
> +                if (elem !== real_target)
> +                    log(elem.id + " should have gotten focus but didn't.");
> +            } else {
> +                if (elem === real_target)
> +                    log(elem.id + " shouldn't have gotten focus but did.");
> +            }

This test should be a script-test. (Use http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test).

[Dave Michael]

Created attachment 122100 [details]
Patch

[Dave Michael]

Created attachment 122105 [details]
Patch

[Dave Michael]

Comment on attachment 122100 [details]
Patch

Thanks for all the comments. I think the patch is in much better shape now.

Note this does not put plugin elements in the default tab-order. We may also want to do that, perhaps in a separate change?

[Dave Michael]

Comment on attachment 122105 [details]
Patch

Sorry, *this* is the right patch. The other was missing a little bit.

[Dave Michael]

Comment on attachment 121906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121906&action=review

>> Source/WebCore/html/HTMLAppletElement.h:46
>> +    virtual bool useFallbackContent() const OVERRIDE { return false; }
> 
> Instead of making HTMLPluginElement:: useFallbackContent a pure virtual, you can move the definition in HTMLPluginImageElement to HTMLPluginElement so that you don't have to add new definition here.

Done.

>> Source/WebCore/html/HTMLPlugInElement.cpp:192
>> +    if (useFallbackContent())
> 
> You should also make sure HTMLFrameOwnerElement::supportsFocus() returns true. e.g. if this plugin is showing a fallback content and has contenteditable content attribute, then this element should be focusible (we need a test case for this). r- because of this.

Done, nice catch. I also added a test to ensure that elements with an explicit tabindex are always focusable.

>> Source/WebCore/html/HTMLPlugInElement.cpp:194
>> +    RenderObject* r = renderer();
> 
> Please don't use one-letter variable. Also, renderer() is an inline function so there's no need to cache it.

Done.

>> Source/WebCore/html/HTMLPlugInElement.cpp:195
>> +    if (!r || !(r->isEmbeddedObject()))
> 
> Redundant parenthesis around r->isEmbeddedObject(). This condition should be re-written as:
> !renderer() || !renderer()->isEmbeddedObject()

I had to change my logic around a bit, since the "applet" tag doesn't use a RenderEmbeddedObject. I think your concern has been addressed.

>> Source/WebCore/html/HTMLPlugInElement.cpp:197
>> +    RenderEmbeddedObject* renderEmbed(toRenderEmbeddedObject(r));
> 
> Please use operator= here: renderEmbed = toRenderEmbeddedObject(renderer()).

Done.

>> Source/WebCore/html/HTMLPlugInElement.cpp:209
>> +            page->focusController()->setFocusedNode(this, contentFrame());
> 
> Could you explain why we need this code? Also, maybe we can share code with HTMLFrameElementBase::setFocus ?

Good catch. I was cargo culting and copied from HTMLFrameElementBase. We don't actually need to override it in this case.

>> LayoutTests/ChangeLog:7
>> +
> 
> Please explain what kind of a test you're adding.

Done.

>> LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
>> +336 focus / 336 blur events dispatched, and should be 336 / 336 PASSED
> 
> Why is this change not listed on LayoutTests/ChangeLog ?

Done.

>> LayoutTests/plugins/focus.html:1
>> +<html>
> 
> Are you intentionally testing quirks mode? If not, we need <!DOCTYPE html> here.

Done.

>> LayoutTests/plugins/focus.html:29
>> +        var elem = owner.childNodes[i];
> 
> Please don't use abbreviations like elem.

Done.

>> LayoutTests/plugins/focus.html:33
>> +            real_target = undefined;
> 
> Please use camelCase.

Done.

>> LayoutTests/plugins/focus.html:41
>> +            }
> 
> This test should be a script-test. (Use http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test).

Done.

[Alexey Proskuryakov]

> Note this does not put plugin elements in the default tab-order. We may also want to do that, perhaps in a separate change?

Why is the change in tabindex-focus-blur-all.js if they are not in tab-order?

I would not expect tab to focus plug-ins unless a property to focus all objects is enabled in Safari or Keyboard preferences.

[Alexey Proskuryakov]

I see now, that test actually calls focus().

[Eric Seidel (no email)]

Comment on attachment 122105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122105&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:192
> +    if (HTMLFrameOwnerElement::supportsFocus())
> +        return true;

Are you intentionally trying to short-circuit the useFallbackConent(), !renderer() checks?  I would think you would write this the opposite:

if (!HTMLFrameOwneerElement::supportsFocus())
    return false;

> Source/WebCore/html/HTMLPlugInElement.cpp:198
> +    if (renderer()->isEmbeddedObject()) {

Should this function return true if isEmbeddedObject() is false?

[Dave Michael]

Created attachment 124819 [details]
Patch

[Dave Michael]

Comment on attachment 122105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122105&action=review

Thanks for looking. I updated the patch to try to explain the logic better. If it is wrong, please call me on it.

Jumping up to a higher level... is there a forum where I should bring up this question to make sure it's okay to add this behavior to WebKit? (I also have a question about making plugin elements keyboard focusable which this patch doesn't address).

>> Source/WebCore/html/HTMLPlugInElement.cpp:192
>> +        return true;
> 
> Are you intentionally trying to short-circuit the useFallbackConent(), !renderer() checks?  I would think you would write this the opposite:
> 
> if (!HTMLFrameOwneerElement::supportsFocus())
>     return false;

This actually goes up the chain one more to HTMLElement, which looks like this:
"""
bool HTMLElement::supportsFocus() const
{
    return Element::supportsFocus() || (rendererIsEditable() && parentNode() && !parentNode()->rendererIsEditable());
}
"""
where the next up the chain looks like this:
"""
bool Node::supportsFocus() const
{
    return hasRareData() && rareData()->tabIndexSetExplicitly();
}
"""

My intent was to force the element to be focusable if tabindex is set explicitly, since the following spec:
http://www.w3.org/TR/html5/editing.html#sequential-focus-navigation-and-the-tabindex-attribute
says (if tabindex is explicitly set):
"The user agent must allow the element to be focused"

>> Source/WebCore/html/HTMLPlugInElement.cpp:198
>> +    if (renderer()->isEmbeddedObject()) {
> 
> Should this function return true if isEmbeddedObject() is false?

No; this check only applies for <embed>, but I was also trying to allow <applet> and <object>. I updated the comments to try to reflect that.

[Early Warning System Bot]

Comment on attachment 124819 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 124819 [details]
Patch

Attachment 124819 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13494509

[Build Bot]

Comment on attachment 124819 [details]
Patch

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

[Kent Tamura]

Comment on attachment 124819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124819&action=review

> Source/WebCore/html/HTMLPlugInElement.h:75
> +    virtual bool useFallbackContent() const { return false; }

So, HTMLPluginImageElement::useFallbackContent should be declared with OVERRIDE.

[Dave Michael]

Created attachment 195758 [details]
Patch

[Dave Michael]

Addressed your comment, merged, & updated a call from pluginCrashedOrWasMissing() to showsUnavailablePluginIndicator.

[Ryosuke Niwa]

Comment on attachment 195758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195758&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:248
> +    // If our parents think we should support focus (e.g., because tabindex is
> +    // set), then we support it (regardless of later checks).

We don't normally add "what" comments like this that just repeats the code.
We add "why" comments.

> Source/WebCore/html/HTMLPlugInElement.cpp:252
> +    // Make plugin elements with fallback not focusable by default.

Ditto.

> Source/WebCore/html/HTMLPlugInElement.cpp:259
> +    // For <embed> elements, we need to check with the renderer object to see if
> +    // fallback content is in use or if the plugin has crashed. This doesn't
> +    // apply for <applet> or <object>.

Ditto.

> LayoutTests/plugins/focus.html:13
> +<embed id="embed_elem" type="application/x-webkit-test-netscape" width=100 height=100 shouldFocus=true></embed>
> +<object id="object_elem" type="application/x-webkit-test-netscape" windowedPlugin="false" width=100 height=100 shouldFocus=true></object>
> +<applet id="applet_elem" width=100 height=100 code="" shouldFocus=true></applet>

I'd prefer we use camelCase.

> LayoutTests/plugins/focus.html:15
> +<!-- We don't support focus() by default when there's a missing plugin. -->

This comment also repeats the test case.
What does this comment say any more than what I can see in the following two lines?
If anything, we should be explaining why we do that. If not, please remove these comments.
They're making the test code harder to read.

[Build Bot]

Comment on attachment 195758 [details]
Patch

Attachment 195758 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17361379

[Dave Michael]

Created attachment 195774 [details]
Patch

[Dave Michael]

Comment on attachment 195758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195758&action=review

>> Source/WebCore/html/HTMLPlugInElement.cpp:248
>> +    // set), then we support it (regardless of later checks).
> 
> We don't normally add "what" comments like this that just repeats the code.
> We add "why" comments.

I added this and the other comments in this file in response to Eric Seidel's questions above.

I rephrased it to try to emphasize the part that I think is not obvious from the code (hopefully explains the "why").

>> Source/WebCore/html/HTMLPlugInElement.cpp:252
>> +    // Make plugin elements with fallback not focusable by default.
> 
> Ditto.

Removed.

>> Source/WebCore/html/HTMLPlugInElement.cpp:259
>> +    // apply for <applet> or <object>.
> 
> Ditto.

I rephrased and moved down to try to emphasize the part that's not obvious from the code.

>> LayoutTests/plugins/focus.html:13
>> +<applet id="applet_elem" width=100 height=100 code="" shouldFocus=true></applet>
> 
> I'd prefer we use camelCase.

Done.

>> LayoutTests/plugins/focus.html:15
>> +<!-- We don't support focus() by default when there's a missing plugin. -->
> 
> This comment also repeats the test case.
> What does this comment say any more than what I can see in the following two lines?
> If anything, we should be explaining why we do that. If not, please remove these comments.
> They're making the test code harder to read.

I thought it was easier to understand that way, but it was admittedly redundant. Removed.

[Ryosuke Niwa]

Comment on attachment 195774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195774&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:248
> +    // We must check with our parent, because if tabindex is set, we must
> +    // support focus.

This comment is inaccurate in that HTMLFrameOwnerElement::supportsFocus() also returns true for a root editable element.
We should either mention that or fix the code. This is also why I’m not sure it’s worth leaving a comment like this.
It’s likely that a comment like this will get outdated as supportsFocus in super classes get modified.

> Source/WebCore/html/HTMLPlugInElement.cpp:262
> +    // If this is not an <embed> element, it could still be an <applet> or
> +    // <object>, which should be focusable.

Why don’t we check renderer for applet or object?
Also, <embed> is not an element. It’s a start tag of an embed element.

> LayoutTests/plugins/focus.html:71
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}

Nit: no curly brackets around a single line statement.

[Build Bot]

Comment on attachment 195774 [details]
Patch

Attachment 195774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17334265

[Dave Michael]

Created attachment 195782 [details]
Patch

[Dave Michael]

Created attachment 195784 [details]
Patch

[Ryosuke Niwa]

I still don’t understand why we don’t have to check renderers for applet and object?

[Dave Michael]

Comment on attachment 195774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195774&action=review

>> Source/WebCore/html/HTMLPlugInElement.cpp:248
>> +    // support focus.
> 
> This comment is inaccurate in that HTMLFrameOwnerElement::supportsFocus() also returns true for a root editable element.
> We should either mention that or fix the code. This is also why I’m not sure it’s worth leaving a comment like this.
> It’s likely that a comment like this will get outdated as supportsFocus in super classes get modified.

The comment was incomplete, I agree.

Are you concerned with the behavior, or just the comment?

>> Source/WebCore/html/HTMLPlugInElement.cpp:262
>> +    // <object>, which should be focusable.
> 
> Why don’t we check renderer for applet or object?
> Also, <embed> is not an element. It’s a start tag of an embed element.

Because they're the only other elements that ultimately inherit HTMLPlugInElement, hence this code applies to them with no reason to explicitly list them. I removed the comment since I suppose it could become out of date if another element type were added in the hierarchy.

>> LayoutTests/plugins/focus.html:71
>> +}
> 
> Nit: no curly brackets around a single line statement.

Done.

[Dave Michael]

Comment on attachment 124819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124819&action=review

>> Source/WebCore/html/HTMLPlugInElement.h:75
>> +    virtual bool useFallbackContent() const { return false; }
> 
> So, HTMLPluginImageElement::useFallbackContent should be declared with OVERRIDE.

This is actually not overriding anything; it can be overridden by children. This just added the function higher in the hierarchy so that we can call it from supportsFocus.

[Ryosuke Niwa]

Comment on attachment 195774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195774&action=review

>>> Source/WebCore/html/HTMLPlugInElement.cpp:262
>>> +    // <object>, which should be focusable.
>> 
>> Why don’t we check renderer for applet or object?
>> Also, <embed> is not an element. It’s a start tag of an embed element.
> 
> Because they're the only other elements that ultimately inherit HTMLPlugInElement, hence this code applies to them with no reason to explicitly list them. I removed the comment since I suppose it could become out of date if another element type were added in the hierarchy.

Upon some research, I realized that isEmbeddedObject() is true for both applet and object elements, meaning that this comment was incorrect.
Is it intentional that we check hasFallbackContent() and showsUnavailablePluginIndicator() for applet and object elements as well?
The test cases you've added seems to indicate that the answer to this question is yes but perhaps we're lacking some in-depth testing here?

[Ryosuke Niwa]

On a completely unrelated note, we should probably rename RenderEmbeddedObject to RenderPlugin to avoid misleading readers to think it's only used for an embed element.

[Ryosuke Niwa]

Comment on attachment 124819 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124819&action=review

>>> Source/WebCore/html/HTMLPlugInElement.h:75
>>> +    virtual bool useFallbackContent() const { return false; }
>> 
>> So, HTMLPluginImageElement::useFallbackContent should be declared with OVERRIDE.
> 
> This is actually not overriding anything; it can be overridden by children. This just added the function higher in the hierarchy so that we can call it from supportsFocus.

You should probably remove the trivial definition of useFallbackContent() from HTMLPlugInImageElement.

[Build Bot]

Comment on attachment 195774 [details]
Patch

Attachment 195774 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17353216

[Build Bot]

Comment on attachment 195784 [details]
Patch

Attachment 195784 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17358139

New failing tests:
plugins/focus.html

[Build Bot]

Created attachment 195804 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 195774 [details]
Patch

Attachment 195774 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17368270

[Dave Michael]

Created attachment 196190 [details]
Patch

[Dave Michael]

Created attachment 196208 [details]
Patch

[Dave Michael]

Comment on attachment 195774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195774&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:258
> +        if (renderEmbed->hasFallbackContent() || renderEmbed->showsUnavailablePluginIndicator())

I think using hasFallbackContent() here is wrong. If the plugin element has fallback content, but is displaying correctly (i.e., fallback content is not being used), it seems it should be focusable.

I removed that part of the check and added a test where the plugin is valid and has fallback content (which is not used).

>>>> Source/WebCore/html/HTMLPlugInElement.cpp:262
>>>> +    // <object>, which should be focusable.
>>> 
>>> Why don’t we check renderer for applet or object?
>>> Also, <embed> is not an element. It’s a start tag of an embed element.
>> 
>> Because they're the only other elements that ultimately inherit HTMLPlugInElement, hence this code applies to them with no reason to explicitly list them. I removed the comment since I suppose it could become out of date if another element type were added in the hierarchy.
> 
> Upon some research, I realized that isEmbeddedObject() is true for both applet and object elements, meaning that this comment was incorrect.
> Is it intentional that we check hasFallbackContent() and showsUnavailablePluginIndicator() for applet and object elements as well?
> The test cases you've added seems to indicate that the answer to this question is yes but perhaps we're lacking some in-depth testing here?

Thanks, sorry I didn't notice that before. That changes things...  I revised my logic a bit. It seems isEmbeddedObject() is more of a sanity check required for the downcast, and if it's not true, it makes sense not to support focus.

I updated my tests to try to cover more cases. Unfortunately, I don't know how to test <applet> locally, so my new test in LayoutTests/java might not be valid yet.

[Ryosuke Niwa]

Comment on attachment 196208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196208&action=review

> Source/WebCore/ChangeLog:6
> +        Make embed, object, and applet elements support focus when appropriate.

This description should appear after Reviewed by line. Also you need a much more detailed explanation of your change
since this sentence simply repeats the bug title.

A good change log describes what caused the bug and how you fixed it.
See http://www.webkit.org/coding/contributing.html#changelogs for an example.

> Source/WebCore/ChangeLog:10
> +        Test: plugins/focus.html, java/focus.html

Wrong format. See other entires with multiple tests.

> Source/WebCore/ChangeLog:14
> +        * html/HTMLPlugInElement.h:

The change log is missing an entry for HTMLPlugInImageElement.
Please mention why you're removing supportsFocus there.

> Source/WebCore/html/HTMLPlugInElement.cpp:256
> +        RenderEmbeddedObject* renderEmbed = toRenderEmbeddedObject(renderer());
> +        if (renderEmbed->showsUnavailablePluginIndicator())
> +            return false;
> +        return true;

We can simply rewrite these 4 lines as:
return !toRenderEmbeddedObject(renderer())->showsUnavailablePluginIndicator();
Remember to remove the curly braces once you've done that.

But really, this function needs to be explained in the change log.
e.g. why we don't focus when we use fallback content?

> LayoutTests/ChangeLog:7
> +        Add/update testing to ensure embed, object, and applet tags now support
> +        focus.

Again, the description like this should appear after Reviewed by line.

> LayoutTests/java/focus.html:62
> +            if (pluginElement.getAttribute("shouldFocus") === "true")
> +                shouldBeEqual(pluginElement.id + " should get focus.", pluginElement, realTarget);
> +            else
> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);

Why don't we just use shouldBe? It seems like duplicating all of this logic is unnecessary.

> LayoutTests/java/focus.html:68
> +            if (pluginElement.getAttribute("shouldFocus") === "true")
> +                shouldBeEqual(pluginElement.id + " should lose focus.", pluginElement, realTarget);
> +            else
> +                shouldBeEqual(pluginElement.id + " should not lose focus.", undefined, realTarget);

Ditto.

> LayoutTests/java/focus.html:76
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

layoutTestController has been renamed to testRunner.
Also, you don't need to call this function since js-test-pre automatically does that.
r- because of this clearly wrong code.

> LayoutTests/plugins/focus.html:68
> +            if (pluginElement.getAttribute("shouldFocus") === "true")
> +                shouldBeEqual(pluginElement.id + " should get focus.", pluginElement, realTarget);
> +            else
> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);
> +            realTarget = undefined;
> +            pluginElement.blur();
> +            if (pluginElement.getAttribute("shouldFocus") === "true")
> +                shouldBeEqual(pluginElement.id + " should lose focus.", pluginElement, realTarget);
> +            else
> +                shouldBeEqual(pluginElement.id + " should not lose focus.", undefined, realTarget);

Ditto about using shouldBe.

> LayoutTests/plugins/focus.html:76
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

Ditto about layoutTestController.

[Dave Michael]

Created attachment 196234 [details]
Patch

[Dave Michael]

Comment on attachment 196208 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196208&action=review

>> Source/WebCore/ChangeLog:6
>> +        Make embed, object, and applet elements support focus when appropriate.
> 
> This description should appear after Reviewed by line. Also you need a much more detailed explanation of your change
> since this sentence simply repeats the bug title.
> 
> A good change log describes what caused the bug and how you fixed it.
> See http://www.webkit.org/coding/contributing.html#changelogs for an example.

Done.

>> Source/WebCore/ChangeLog:10
>> +        Test: plugins/focus.html, java/focus.html
> 
> Wrong format. See other entires with multiple tests.

Done.

>> Source/WebCore/ChangeLog:14
>> +        * html/HTMLPlugInElement.h:
> 
> The change log is missing an entry for HTMLPlugInImageElement.
> Please mention why you're removing supportsFocus there.

Done (except I think you meant I should mention why I'm adding it?). Updated to include HTMLPlugInImageElement.

>> Source/WebCore/html/HTMLPlugInElement.cpp:256
>> +        return true;
> 
> We can simply rewrite these 4 lines as:
> return !toRenderEmbeddedObject(renderer())->showsUnavailablePluginIndicator();
> Remember to remove the curly braces once you've done that.
> 
> But really, this function needs to be explained in the change log.
> e.g. why we don't focus when we use fallback content?

Done.

>> LayoutTests/ChangeLog:7
>> +        focus.
> 
> Again, the description like this should appear after Reviewed by line.

Done.

>> LayoutTests/java/focus.html:62
>> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);
> 
> Why don't we just use shouldBe? It seems like duplicating all of this logic is unnecessary.

shouldBe only accepts 2 strings that it evals, which has two consequences:
1) I have to make pluginElement a global (not a big problem, just annoying).
2) There's no "context" field, which means I lose useful information (the id and whether we should or should not get focus).

Using shouldBe, I would get:
PASS pluginElement is realTarget
PASS pluginElement is realTarget
PASS undefined is realTarget
PASS undefined is realTarget
instead of something like:
PASS objectElemWithFallbackContents should get focus.
PASS objectElemWithFallbackContents should lose focus.
PASS noPluginEmbedElem should not get focus.
PASS noPluginEmbedElem should not lose focus.

I find the latter more readable and easier to debug.

>> LayoutTests/java/focus.html:76
>> +    layoutTestController.dumpAsText();
> 
> layoutTestController has been renamed to testRunner.
> Also, you don't need to call this function since js-test-pre automatically does that.
> r- because of this clearly wrong code.

Sorry, I'd let this patch sit idle for a long time. Didn't realize this changed. Done.

>> LayoutTests/plugins/focus.html:76
>> +    layoutTestController.dumpAsText();
> 
> Ditto about layoutTestController.

Done.

[Early Warning System Bot]

Comment on attachment 196234 [details]
Patch

Attachment 196234 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17399044

[Build Bot]

Comment on attachment 196234 [details]
Patch

Attachment 196234 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17406119

[Early Warning System Bot]

Comment on attachment 196234 [details]
Patch

Attachment 196234 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17383306

[Dave Michael]

Created attachment 196238 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 196238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196238&action=review

> Source/WebCore/ChangeLog:16
> +        Test: plugins/focus.html
> +        Test: java/focus.html

Wrong format again. It should look like this:
Test: plugins/focus.html
      java/focus.html

> LayoutTests/java/focus.html:38
> +var realTarget;
> +
> +function FocusEventHandler(evnt) {
> +    realTarget = evnt.currentTarget;
> +}

On my second thought, we don't need this function.

> LayoutTests/java/focus.html:54
> +            pluginElement.addEventListener("focus", FocusEventHandler, false);
> +            pluginElement.addEventListener("blur", FocusEventHandler, false);

Or these event handlers.

> LayoutTests/java/focus.html:60
> +            pluginElement.focus();
> +            if (pluginElement.getAttribute("shouldFocus") === "true")
> +                shouldBeEqual(pluginElement.id + " should get focus.", pluginElement, realTarget);
> +            else
> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);

A better way of writing this test will be:
shouldBe('pluginElement.focus();document.activeElement == pluginElement', 'pluginElement.getAttribute("shouldFocus")');

[Ryosuke Niwa]

Comment on attachment 196238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196238&action=review

>> LayoutTests/java/focus.html:60
>> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);
> 
> A better way of writing this test will be:
> shouldBe('pluginElement.focus();document.activeElement == pluginElement', 'pluginElement.getAttribute("shouldFocus")');

I'd add at much better approach will be something like this:
shouldBe('pluginElement = document.getElementById("' + pluginElement.id + '");pluginElement.focus();document.activeElement == pluginElement', 'pluginElement.getAttribute("shouldFocus")');

[Ryosuke Niwa]

The point here is to reduce the code in tests and make the output self-evident as to what each test case is testing and what the correct output is.

[WebKit Review Bot]

Comment on attachment 196238 [details]
Patch

Attachment 196238 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17316454

New failing tests:
fast/events/tabindex-focus-blur-all.html

[WebKit Review Bot]

Created attachment 196251 [details]
Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Dave Michael]

Created attachment 196365 [details]
Patch

[Dave Michael]

Created attachment 196381 [details]
Patch

[Dave Michael]

Comment on attachment 196238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196238&action=review

>> Source/WebCore/ChangeLog:16
>> +        Test: java/focus.html
> 
> Wrong format again. It should look like this:
> Test: plugins/focus.html
>       java/focus.html

Sorry, I scrolled down for an example and the first thing I saw was the wrong way.

> LayoutTests/fast/events/resources/tabindex-focus-blur-all.js:21
> +                             "OBJECT",

I decided on a different approach that I think is more in keeping with the spirit of this test. Embedded elements are still not always focusable with my patch, so I changed EMBED and OBJECT to use a non-existent plugin. Applet was already using empty "code". Now these elements won't be focused in this test except when the tabindex is set.

>>> LayoutTests/java/focus.html:60
>>> +                shouldBeEqual(pluginElement.id + " should not get focus.", undefined, realTarget);
>> 
>> A better way of writing this test will be:
>> shouldBe('pluginElement.focus();document.activeElement == pluginElement', 'pluginElement.getAttribute("shouldFocus")');
> 
> I'd add at much better approach will be something like this:
> shouldBe('pluginElement = document.getElementById("' + pluginElement.id + '");pluginElement.focus();document.activeElement == pluginElement', 'pluginElement.getAttribute("shouldFocus")');

Done. Thanks, that's definitely shorter and still has helpful output.

[Ryosuke Niwa]

Comment on attachment 196381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196381&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:252
> +    if (useFallbackContent())
> +        return false;
> +    if (renderer() && renderer()->isEmbeddedObject())

Maybe we can combine these two conditions like this:
if (useFallbackContent() || !renderer() || !renderer()->isEmbeddedObject())
    return false;

> LayoutTests/ChangeLog:11
> +        * fast/events/resources/tabindex-focus-blur-all-frame1.html: Change embed and object elements to reference an invalid plugin, to be consistent with applet. These element types are not focusable when there is valid plugin content.

Nit: please wrap this line at some point. It's too long.

> LayoutTests/java/focus-expected.txt:9
> +PASS pluginElement = document.getElementById("appletElem"); pluginElement.focus(); document.activeElement === pluginElement is pluginElement.getAttribute("shouldFocus") === "true"

Nit: Perhaps we can define $ = function (id) { return document.getElementById(id); } or something to shorten the line here.

> LayoutTests/plugins/focus.html:40
> +                     'pluginElement.getAttribute("shouldFocus") === "true"');

Nit: wrong indentation. 'pluginElement.getAttribute' should be exactly 4 spaces to the right of shouldBe(.

[Ryosuke Niwa]

Comment on attachment 196381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196381&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Ryosuke Niwa.

By the way, please don't replace NOBODY (OOPS!) by a reviewer's name until the reviewer gave your patch r+.

[Dave Michael]

Created attachment 196405 [details]
Patch

[Dave Michael]

Comment on attachment 196381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196381&action=review

Thanks very much for the thoughtful and speedy reviews!

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by Ryosuke Niwa.
> 
> By the way, please don't replace NOBODY (OOPS!) by a reviewer's name until the reviewer gave your patch r+.

Oops, sorry. Wasn't sure when I was supposed to do that; see now that I shouldn't have.

>> Source/WebCore/html/HTMLPlugInElement.cpp:252
>> +    if (renderer() && renderer()->isEmbeddedObject())
> 
> Maybe we can combine these two conditions like this:
> if (useFallbackContent() || !renderer() || !renderer()->isEmbeddedObject())
>     return false;

Sure, done.

>> LayoutTests/ChangeLog:11
>> +        * fast/events/resources/tabindex-focus-blur-all-frame1.html: Change embed and object elements to reference an invalid plugin, to be consistent with applet. These element types are not focusable when there is valid plugin content.
> 
> Nit: please wrap this line at some point. It's too long.

Done.

>> LayoutTests/java/focus-expected.txt:9
>> +PASS pluginElement = document.getElementById("appletElem"); pluginElement.focus(); document.activeElement === pluginElement is pluginElement.getAttribute("shouldFocus") === "true"
> 
> Nit: Perhaps we can define $ = function (id) { return document.getElementById(id); } or something to shorten the line here.

I tried a slightly different approach that makes the output pretty short but still readable, including the element id. Let me know what you think.

>> LayoutTests/plugins/focus.html:40
>> +                     'pluginElement.getAttribute("shouldFocus") === "true"');
> 
> Nit: wrong indentation. 'pluginElement.getAttribute' should be exactly 4 spaces to the right of shouldBe(.

Done.

[Ryosuke Niwa]

Comment on attachment 196405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196405&action=review

> Source/WebCore/html/HTMLPlugInElement.cpp:252
> +    if (HTMLFrameOwnerElement::supportsFocus())
> +        return true;
> +
> +    if (useFallbackContent() || !renderer() || !renderer()->isEmbeddedObject())
> +        return false;
> +    return !toRenderEmbeddedObject(renderer())->showsUnavailablePluginIndicator();

OMG! This function is so self-evidently clear now.

> LayoutTests/plugins/focus-expected.txt:9
> +PASS "embedElem"; document.activeElement === pluginElement is true

I guess the old output was nice in that we could see what pluginElement is set to but I guess it's obvious from the context.

[WebKit Review Bot]

Comment on attachment 196405 [details]
Patch

Rejecting attachment 196405 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 196405 [details]
Patch

Clearing flags on attachment: 196405

Committed r147591: <http://trac.webkit.org/changeset/147591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
Need a rebaseline or real failure?

[Dave Michael]

(In reply to comment #71)
> This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
> Need a rebaseline or real failure?
I'm sorry, I'm not sure where to look to see the failures. It passed on the trybots. Is it a pixel difference? That would just require a rebaseline. The test looks like it's passing this morning; maybe somebody already rebaselined?

Sorry for any inconvenience.

[Jer Noble]

(In reply to comment #72)
> (In reply to comment #71)
> > This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
> > Need a rebaseline or real failure?
> I'm sorry, I'm not sure where to look to see the failures. It passed on the trybots. Is it a pixel difference? That would just require a rebaseline. The test looks like it's passing this morning; maybe somebody already rebaselined?

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r147640%20(7636)/results.html

The relevant section of the failure diff:

+applet2 FAILED - was focused but focus wasn't expected
+elemThatShouldFocus is null, focusedElem is <APPLET> applet2

[Dave Michael]

(In reply to comment #73)
> (In reply to comment #72)
> > (In reply to comment #71)
> > > This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
> > > Need a rebaseline or real failure?
> > I'm sorry, I'm not sure where to look to see the failures. It passed on the trybots. Is it a pixel difference? That would just require a rebaseline. The test looks like it's passing this morning; maybe somebody already rebaselined?
> 
> http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r147640%20(7636)/results.html
> 
> The relevant section of the failure diff:
> 
> +applet2 FAILED - was focused but focus wasn't expected
> +elemThatShouldFocus is null, focusedElem is <APPLET> applet2
Thank you! I think I know what's going on, but I'm not sure how to test my attempted fix on Linux. Is there a way I can try this on a Apple MountainLion Release WK2 trybot? Or could somebody else try for me? I uploaded my speculative fix here:
https://bugs.webkit.org/show_bug.cgi?id=32292

[Dave Michael]

(In reply to comment #74)
> (In reply to comment #73)
> > (In reply to comment #72)
> > > (In reply to comment #71)
> > > > This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
> > > > Need a rebaseline or real failure?
> > > I'm sorry, I'm not sure where to look to see the failures. It passed on the trybots. Is it a pixel difference? That would just require a rebaseline. The test looks like it's passing this morning; maybe somebody already rebaselined?
> > 
> > http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r147640%20(7636)/results.html
> > 
> > The relevant section of the failure diff:
> > 
> > +applet2 FAILED - was focused but focus wasn't expected
> > +elemThatShouldFocus is null, focusedElem is <APPLET> applet2
> Thank you! I think I know what's going on, but I'm not sure how to test my attempted fix on Linux. Is there a way I can try this on a Apple MountainLion Release WK2 trybot? Or could somebody else try for me? I uploaded my speculative fix here:
> https://bugs.webkit.org/show_bug.cgi?id=32292
Sorry, I meant here:
https://bugs.webkit.org/show_bug.cgi?id=113950

[Jer Noble]

(In reply to comment #75)
> Sorry, I meant here:
> https://bugs.webkit.org/show_bug.cgi?id=113950

I'll give it a try.