Bug 32292 - Unable to focus on embedded plugins such as Flash via javascript focus()
Summary: Unable to focus on embedded plugins such as Flash via javascript focus()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Dave Michael
URL:
Keywords:
Depends on:
Blocks: 112992 113928 113960
  Show dependency treegraph
 
Reported: 2009-12-08 15:02 PST by Dave Myron
Modified: 2013-04-04 14:14 PDT (History)
14 users (show)

See Also:


Attachments
Patch (4.67 KB, patch)
2012-01-06 15:19 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2012-01-10 14:00 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (9.09 KB, patch)
2012-01-10 14:09 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2012-01-11 14:57 PST, Dave Michael
dmichael: review-
dmichael: commit-queue-
Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2012-01-11 15:09 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2012-01-31 14:21 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2013-03-29 10:01 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (10.71 KB, patch)
2013-03-29 12:01 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2013-03-29 12:58 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (10.49 KB, patch)
2013-03-29 13:12 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (595.98 KB, application/zip)
2013-03-29 14:47 PDT, Build Bot
no flags Details
Patch (16.56 KB, patch)
2013-04-02 11:20 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2013-04-02 12:26 PDT, Dave Michael
dmichael: review-
Details | Formatted Diff | Diff
Patch (17.97 KB, patch)
2013-04-02 15:10 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (17.97 KB, patch)
2013-04-02 15:23 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64 (1.03 MB, application/zip)
2013-04-02 16:29 PDT, WebKit Review Bot
no flags Details
Patch (17.73 KB, patch)
2013-04-03 09:58 PDT, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (20.03 KB, patch)
2013-04-03 11:47 PDT, Dave Michael
dmichael: review+
Details | Formatted Diff | Diff
Patch (17.99 KB, patch)
2013-04-03 13:54 PDT, Dave Michael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Myron 2009-12-08 15:02:12 PST
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.
Comment 1 Paul Dragoonis 2010-05-13 07:09:36 PDT
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.
Comment 2 Alexey Proskuryakov 2010-05-14 17:43:38 PDT
HTMLPlugInImageElement probably needs a custom setFocus() implementation, similar to HTMLFrameElementBase.
Comment 3 Sunil 2010-08-14 08:36:26 PDT
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.
Comment 4 Dave Michael 2012-01-06 15:19:53 PST
Created attachment 121507 [details]
Patch
Comment 5 Adam Barth 2012-01-06 15:25:35 PST
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)?
Comment 6 Ryosuke Niwa 2012-01-06 15:35:20 PST
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.
Comment 7 WebKit Review Bot 2012-01-06 16:41:39 PST
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
Comment 8 Dave Michael 2012-01-10 14:00:53 PST
Created attachment 121904 [details]
Patch
Comment 9 Dave Michael 2012-01-10 14:09:58 PST
Created attachment 121906 [details]
Patch
Comment 10 Ryosuke Niwa 2012-01-10 14:46:48 PST
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).
Comment 11 Dave Michael 2012-01-11 14:57:46 PST
Created attachment 122100 [details]
Patch
Comment 12 Dave Michael 2012-01-11 15:09:16 PST
Created attachment 122105 [details]
Patch
Comment 13 Dave Michael 2012-01-11 15:13:14 PST
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?
Comment 14 Dave Michael 2012-01-11 15:14:04 PST
Comment on attachment 122105 [details]
Patch

Sorry, *this* is the right patch. The other was missing a little bit.
Comment 15 Dave Michael 2012-01-11 15:15:40 PST
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.
Comment 16 Alexey Proskuryakov 2012-01-11 15:20:47 PST
> 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.
Comment 17 Alexey Proskuryakov 2012-01-11 15:22:38 PST
I see now, that test actually calls focus().
Comment 18 Eric Seidel (no email) 2012-01-30 16:26:02 PST
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?
Comment 19 Dave Michael 2012-01-31 14:21:26 PST
Created attachment 124819 [details]
Patch
Comment 20 Dave Michael 2012-01-31 14:25:10 PST
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.
Comment 21 Early Warning System Bot 2012-03-07 19:09:28 PST
Comment on attachment 124819 [details]
Patch

Attachment 124819 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11861080
Comment 22 Peter Beverloo (cr-android ews) 2012-08-14 05:03:14 PDT
Comment on attachment 124819 [details]
Patch

Attachment 124819 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13494509
Comment 23 Build Bot 2013-01-17 00:55:48 PST
Comment on attachment 124819 [details]
Patch

Attachment 124819 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15907709
Comment 24 Kent Tamura 2013-03-28 23:31:37 PDT
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.
Comment 25 Dave Michael 2013-03-29 10:01:24 PDT
Created attachment 195758 [details]
Patch
Comment 26 Dave Michael 2013-03-29 10:05:24 PDT
Addressed your comment, merged, & updated a call from pluginCrashedOrWasMissing() to showsUnavailablePluginIndicator.
Comment 27 Ryosuke Niwa 2013-03-29 10:05:55 PDT
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.
Comment 28 Build Bot 2013-03-29 10:43:22 PDT
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
Comment 29 Dave Michael 2013-03-29 12:01:50 PDT
Created attachment 195774 [details]
Patch
Comment 30 Dave Michael 2013-03-29 12:10:23 PDT
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.
Comment 31 Ryosuke Niwa 2013-03-29 12:17:43 PDT
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.
Comment 32 Build Bot 2013-03-29 12:39:45 PDT
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
Comment 33 Dave Michael 2013-03-29 12:58:36 PDT
Created attachment 195782 [details]
Patch
Comment 34 Dave Michael 2013-03-29 13:12:22 PDT
Created attachment 195784 [details]
Patch
Comment 35 Ryosuke Niwa 2013-03-29 13:20:26 PDT
I still don’t understand why we don’t have to check renderers for applet and object?
Comment 36 Dave Michael 2013-03-29 13:57:51 PDT
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.
Comment 37 Dave Michael 2013-03-29 14:05:21 PDT
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.
Comment 38 Ryosuke Niwa 2013-03-29 14:08:55 PDT
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?
Comment 39 Ryosuke Niwa 2013-03-29 14:10:45 PDT
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.
Comment 40 Ryosuke Niwa 2013-03-29 14:19:49 PDT
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.
Comment 41 Build Bot 2013-03-29 14:44:27 PDT
Comment on attachment 195774 [details]
Patch

Attachment 195774 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17353216
Comment 42 Build Bot 2013-03-29 14:47:44 PDT
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
Comment 43 Build Bot 2013-03-29 14:47:47 PDT
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
Comment 44 Build Bot 2013-04-01 20:03:06 PDT
Comment on attachment 195774 [details]
Patch

Attachment 195774 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17368270
Comment 45 Dave Michael 2013-04-02 11:20:59 PDT
Created attachment 196190 [details]
Patch
Comment 46 Dave Michael 2013-04-02 12:26:06 PDT
Created attachment 196208 [details]
Patch
Comment 47 Dave Michael 2013-04-02 12:32:34 PDT
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.
Comment 48 Ryosuke Niwa 2013-04-02 12:52:31 PDT
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.
Comment 49 Dave Michael 2013-04-02 15:10:41 PDT
Created attachment 196234 [details]
Patch
Comment 50 Dave Michael 2013-04-02 15:11:33 PDT
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.
Comment 51 Early Warning System Bot 2013-04-02 15:18:51 PDT
Comment on attachment 196234 [details]
Patch

Attachment 196234 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17399044
Comment 52 Build Bot 2013-04-02 15:20:20 PDT
Comment on attachment 196234 [details]
Patch

Attachment 196234 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17406119
Comment 53 Early Warning System Bot 2013-04-02 15:21:06 PDT
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
Comment 54 Dave Michael 2013-04-02 15:23:40 PDT
Created attachment 196238 [details]
Patch
Comment 55 Ryosuke Niwa 2013-04-02 15:33:12 PDT
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")');
Comment 56 Ryosuke Niwa 2013-04-02 15:35:22 PDT
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")');
Comment 57 Ryosuke Niwa 2013-04-02 15:36:04 PDT
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.
Comment 58 WebKit Review Bot 2013-04-02 16:29:28 PDT
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
Comment 59 WebKit Review Bot 2013-04-02 16:29:33 PDT
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
Comment 60 Dave Michael 2013-04-03 09:58:51 PDT
Created attachment 196365 [details]
Patch
Comment 61 Dave Michael 2013-04-03 11:47:41 PDT
Created attachment 196381 [details]
Patch
Comment 62 Dave Michael 2013-04-03 11:52:11 PDT
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.
Comment 63 Ryosuke Niwa 2013-04-03 12:30:13 PDT
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(.
Comment 64 Ryosuke Niwa 2013-04-03 12:31:18 PDT
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+.
Comment 65 Dave Michael 2013-04-03 13:54:52 PDT
Created attachment 196405 [details]
Patch
Comment 66 Dave Michael 2013-04-03 13:57:05 PDT
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.
Comment 67 Ryosuke Niwa 2013-04-03 14:02:33 PDT
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.
Comment 68 WebKit Review Bot 2013-04-03 14:14:38 PDT
Comment on attachment 196405 [details]
Patch

Rejecting attachment 196405 [details] from commit-queue.

dmichael@chromium.org 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.
Comment 69 WebKit Review Bot 2013-04-03 15:34:02 PDT
Comment on attachment 196405 [details]
Patch

Clearing flags on attachment: 196405

Committed r147591: <http://trac.webkit.org/changeset/147591>
Comment 70 WebKit Review Bot 2013-04-03 15:34:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 71 Benjamin Poulain 2013-04-03 16:33:51 PDT
This causes fast/events/tabindex-focus-blur-all.html to fail on the bots.
Need a rebaseline or real failure?
Comment 72 Dave Michael 2013-04-04 08:45:14 PDT
(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.
Comment 73 Jer Noble 2013-04-04 11:19:59 PDT
(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
Comment 74 Dave Michael 2013-04-04 12:47:09 PDT
(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
Comment 75 Dave Michael 2013-04-04 12:47:46 PDT
(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
Comment 76 Jer Noble 2013-04-04 13:07:51 PDT
(In reply to comment #75)
> Sorry, I meant here:
> https://bugs.webkit.org/show_bug.cgi?id=113950

I'll give it a try.