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.
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.
HTMLPlugInImageElement probably needs a custom setFocus() implementation, similar to HTMLFrameElementBase.
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.
Created attachment 121507 [details] Patch
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 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 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
Created attachment 121904 [details] Patch
Created attachment 121906 [details] Patch
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).
Created attachment 122100 [details] Patch
Created attachment 122105 [details] Patch
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 on attachment 122105 [details] Patch Sorry, *this* is the right patch. The other was missing a little bit.
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.
> 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.
I see now, that test actually calls focus().
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?
Created attachment 124819 [details] Patch
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 on attachment 124819 [details] Patch Attachment 124819 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11861080
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 on attachment 124819 [details] Patch Attachment 124819 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15907709
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.
Created attachment 195758 [details] Patch
Addressed your comment, merged, & updated a call from pluginCrashedOrWasMissing() to showsUnavailablePluginIndicator.
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 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
Created attachment 195774 [details] Patch
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 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 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
Created attachment 195782 [details] Patch
Created attachment 195784 [details] Patch
I still don’t understand why we don’t have to check renderers for applet and object?
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 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 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?
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 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 on attachment 195774 [details] Patch Attachment 195774 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17353216
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
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 on attachment 195774 [details] Patch Attachment 195774 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17368270
Created attachment 196190 [details] Patch
Created attachment 196208 [details] Patch
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 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.
Created attachment 196234 [details] Patch
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 on attachment 196234 [details] Patch Attachment 196234 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17399044
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 on attachment 196234 [details] Patch Attachment 196234 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17383306
Created attachment 196238 [details] Patch
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 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")');
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 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
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
Created attachment 196365 [details] Patch
Created attachment 196381 [details] Patch
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 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 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+.
Created attachment 196405 [details] Patch
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 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 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 on attachment 196405 [details] Patch Clearing flags on attachment: 196405 Committed r147591: <http://trac.webkit.org/changeset/147591>
All reviewed patches have been landed. Closing bug.
This causes fast/events/tabindex-focus-blur-all.html to fail on the bots. Need a rebaseline or real failure?
(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.
(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
(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
(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
(In reply to comment #75) > Sorry, I meant here: > https://bugs.webkit.org/show_bug.cgi?id=113950 I'll give it a try.