Don't dispatch blur/focus events if the element's page is not focused.

Don't dispatch blur/focus events if the element's page is not focused.

When the page loses focus a blur event is dispatched once, prevent a second dispatch (via e.g. programmatic blur()-ing), when a page doesn't have focus, prevent a focus event dispatch (via e.g. programmatic focus()-ing), as it will receive a focus event when the page regains focus.

R=tkent@chromium.org
BUG=276757
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160036

[ljs, 2013-10-05 20:27:06]

PTAL

Apologies if I have made mistakes here, this is my first chromium/blink patch
:-)

I have run the layout tests and interactive_ui_tests, however I may have missed
something/missed required tests - could somebody with the appropriate access
send this to a trybot?

Thanks!

[tkent, 2013-10-07 04:49:38]

This doesn't look a correct fix because focus and blur don't match in the
following case:

1. A page gets inactive
2. An element in the page gets focus programmatically.
  -> 'focus' event is dispatched.
3. The element looses focus programmatically.
  -> 'blur' event isn't dispatched even though the element lost focus.

We need to investigate other browser behavior, and to decide what we do.

[ljs, 2013-10-07 18:50:44]

PTAL

I have updated the patch to disallow dispatch of focus events as well as
blur events when the page is active which eliminates this issue with
asymmetric focus/blur event dispatch.

Firefox appears to behave this way under the same circumstances - honouring
the programmatic focus/blur by changing the focus, but not dispatching
events unless the page is active.

I wrote a js fiddle which demonstrates this -
http://jsfiddle.net/eHeez/8/which is a slightly modified version of
your js fiddle -
http://jsfiddle.net/int32_t/FgmSG/7/ the patch appears to cause chromium to
behave the same as firefox in both situations.

On 7 October 2013 05:49, <tkent@chromium.org> wrote:

> This doesn't look a correct fix because focus and blur don't match in the
> following case:
>
> 1. A page gets inactive
> 2. An element in the page gets focus programmatically.
>   -> 'focus' event is dispatched.
> 3. The element looses focus programmatically.
>   -> 'blur' event isn't dispatched even though the element lost focus.
>
> We need to investigate other browser behavior, and to decide what we do.
>
>
>
https://codereview.chromium.**org/26149002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tkent, 2013-10-08 05:39:40]

> I have updated the patch to disallow dispatch of focus events as well as
> blur events when the page is active which eliminates this issue with
> asymmetric focus/blur event dispatch.
> 
> Firefox appears to behave this way under the same circumstances - honouring
> the programmatic focus/blur by changing the focus, but not dispatching
> events unless the page is active.

What about IE?

[ljs, 2013-10-08 07:15:30]

On 8 October 2013 06:39, <tkent@chromium.org> wrote:

> What about IE?
>

 Both IE10 and 11 Release Preview do not dispatch events when the page
doesn't have focus like firefox, however unlike firefox they don't change
the focused element.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tkent, 2013-10-08 08:36:50]

On 2013/10/08 07:15:30, ljs wrote:
> On 8 October 2013 06:39, <mailto:tkent@chromium.org> wrote:
> 
> > What about IE?
> >
> 
>  Both IE10 and 11 Release Preview do not dispatch events when the page
> doesn't have focus like firefox, however unlike firefox they don't change
> the focused element.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Thanks.  I think the Firefox way is simple and the approach of Patch Set 2 looks
good.
We need a test. Probably we had better add a test to
Source/web/tests/WebViewTest.cpp.

[tkent, 2013-10-08 08:41:43]

https://codereview.chromium.org/26149002/diff/5001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/26149002/diff/5001/Source/core/dom/Document.c...
Source/core/dom/Document.cpp:3311: Page* oldPage =
oldFocusedElement->document().page();
page() is enough because oldFocusedElement belongs to this document.

https://codereview.chromium.org/26149002/diff/5001/Source/core/dom/Document.c...
Source/core/dom/Document.cpp:3361: if (!page() ||
page()->focusController().isFocused()) {
!page() is inconsistent with the blur handling above.
How other browsers work with documents without pages?

[tkent, 2013-10-08 08:45:42]

Also you need to sign Individual Contributor License Agreement or Corporate
Contributor License Agreement, and should be added to AUTHORS file.
See http://www.chromium.org/developers/contributing-code

[ljs, 2013-10-08 11:41:23]

On 8 October 2013 09:41, <tkent@chromium.org> wrote:

>
> https://codereview.chromium.**org/26149002/diff/5001/Source/**
>
core/dom/Document.cpp#**newcode3361<https://codereview.chromium.org/26149002/diff/5001/Source/core/dom/Document.cpp#newcode3361>
> Source/core/dom/Document.cpp:**3361: if (!page() ||
> page()->focusController().**isFocused()) {
> !page() is inconsistent with the blur handling above.
> How other browsers work with documents without pages?
>
>
I added this as I wasn't certain what I should do in the case of no page()
+ wanted to retain previous behaviour, however as you rightly point out
that does introduce an inconsistency between focus/blur so the question is
whether to make each consistent in dispatching events when there is no
page, or not dispatching events when there is no page.

I noticed at Document.cpp:1364:-

    // The visibility of the document is inherited from the visibility of
the
    // page. If there is no page associated with the document, we will
assume
    // that the page is hidden, as specified by the spec:
    //
http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/PageVisibility/Overview.html...

Which suggests that a lack of a page() should result in the same behaviour
as the page not being focused.

I'm not certain quite how I would compare this to behaviour in other
browsers, though I will investigate; I could experiment with the behaviour
in chrome in background tabs, however.

I will go ahead and patch the code to not run in the case of !page() for
the time being.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ljs, 2013-10-08 11:48:39]

Apologies, looks like I pulled in some changes from a rebase as well as the
code changes mentioned above in patch set 3. I have now done a second
rebase in patch set 4 to bring the code up to date.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ljs, 2013-10-08 11:54:07]

On 2013/10/08 08:45:42, tkent wrote:
> Also you need to sign Individual Contributor License Agreement or Corporate
> Contributor License Agreement, and should be added to AUTHORS file.
> See http://www.chromium.org/developers/contributing-code

I guess I need to create a new patch for the AUTHORS change + add you as a
reviewer?

[tkent, 2013-10-08 22:47:19]

I found dispatchEventsOnWindowAndFocusedNode in FocusController.cpp didn't
dispatch focusout/DOMFocusOut/focusin/DOMFocusIn events.  We need to fix it
before this CL.

https://codereview.chromium.org/26149002/diff/18001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/26149002/diff/18001/Source/core/dom/Document....
Source/core/dom/Document.cpp:3309: if (page() &&
page()->focusController().isFocused()) {
I remembered no elements could be focusable if a document is detached from a
page. So page() should be always non-null here.  The code looks ok.

[tkent, 2013-10-08 22:47:56]

On 2013/10/08 11:54:07, ljs wrote:
> I guess I need to create a new patch for the AUTHORS change + add you as a
> reviewer?

Yes. You need to make another change for AUTHORS.

[ljs, 2013-10-08 22:56:21]

On 2013/10/08 22:47:19, tkent wrote:
> I found dispatchEventsOnWindowAndFocusedNode in FocusController.cpp didn't
> dispatch focusout/DOMFocusOut/focusin/DOMFocusIn events.  We need to fix it
> before this CL.

Should this be done in a separate CL, or is it ok to attempt this as part of
this one?

[tkent, 2013-10-08 23:32:41]

> Should this be done in a separate CL, or is it ok to attempt this as part of
> this one?

I recommend a separated CL.

[ljs, 2013-10-10 02:40:40]

PTAL.

I have added a test, and rebased against latest blink.

[tkent, 2013-10-10 05:17:19]

lgtm

[commit-bot: I haz the power, 2013-10-10 05:17:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/28001

[commit-bot: I haz the power, 2013-10-10 06:27:24]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[tkent, 2013-10-10 07:33:04]

On 2013/10/10 06:27:24, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Unfortunately some tests failed with this patch.
Please investigate failures.  You can run them by
% ./third_party/WebKit/Tools/Scripts/run-webkit-tests --debug
inspector/editor/text-editor-autocomplete.html



inspector/editor/text-editor-autocomplete.html
inspector/editor/text-editor-enter-behaviour.html
inspector/editor/text-editor-formatter.html
inspector/editor/text-editor-home-button.html
inspector/editor/text-editor-selection-to-search.html
inspector/editor/text-editor-smart-braces.html
inspector/editor/text-editor-word-jumps.html
inspector/elements/edit-dom-actions.html
inspector/elements/edit-trimmed-attribute-value.html
inspector/styles/styles-add-blank-property.html
inspector/styles/styles-add-new-rule.html
inspector/styles/styles-change-node-while-editing.html
inspector/styles/undo-add-rule-crash.html

[ljs, 2013-10-10 23:29:04]

On 2013/10/10 07:33:04, tkent wrote:
> On 2013/10/10 06:27:24, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> 
> Unfortunately some tests failed with this patch.
> Please investigate failures.

PTAL. I have fixed the failures, and I ran all of the layout tests and compared
them vs. results without the patch and no new failures are introduced by the
patch. I also checked the webkit unit tests and they all passed (including of
course the test I added for this patch.)

It turns out that it's feasible for a page to never be focused, in which case
the assumption that events will be dispatched appropriately on refocus turns out
to be false. The inspector appears to rely on this. The solution I implemented
is to track whether the page has ever been focused, if not we go ahead +
dispatch events.

[ljs, 2013-10-10 23:31:16]

On 2013/10/10 23:29:04, ljs wrote:
> It turns out that it's feasible for a page to never be focused, in which case
> the assumption that events will be dispatched appropriately on refocus turns
out
> to be false. The inspector appears to rely on this. The solution I implemented
> is to track whether the page has ever been focused, if not we go ahead +
> dispatch events.

Addendum: The more important broken assumption being that a blur even will have
been dispatched; if the page has never had focus then
dispatchEventsOnWindowAndFocusedNode will never have been called and thus us not
dispatching blur events is not appropriate.

[commit-bot: I haz the power, 2013-10-11 01:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/42001

[commit-bot: I haz the power, 2013-10-11 01:59:51]

Failed to apply patch for Source/core/dom/Document.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Document.cpp
  Hunk #1 FAILED at 147.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/dom/Document.cpp.rej

Patch:       Source/core/dom/Document.cpp
Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index
dcc0051d90f62ffb069fddf961f43544e4b19299..7cc46e2de6ac635bfaae977b6f262ff5c4d65e79
100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -147,6 +147,7 @@
 #include "core/page/DOMSecurityPolicy.h"
 #include "core/page/DOMWindow.h"
 #include "core/page/EventHandler.h"
+#include "core/page/FocusController.h"
 #include "core/page/Frame.h"
 #include "core/page/FrameTree.h"
 #include "core/page/FrameView.h"
@@ -3289,23 +3290,26 @@ bool Document::setFocusedElement(PassRefPtr<Element>
prpNewFocusedElement, Focus
             oldFocusedElement->dispatchFormControlChangeEvent();
 
         // Dispatch the blur event and let the node do any other blur related
activities (important for text fields)
-        oldFocusedElement->dispatchBlurEvent(newFocusedElement.get());
-
-        if (m_focusedElement) {
-            // handler shifted focus
-            focusChangeBlocked = true;
-            newFocusedElement = 0;
-        }
+        // If page lost focus, blur event will have already been dispatched
+        if (page() && (page()->focusController().isFocused() ||
!page()->focusController().beenFocused())) {
+            oldFocusedElement->dispatchBlurEvent(newFocusedElement.get());
+
+            if (m_focusedElement) {
+                // handler shifted focus
+                focusChangeBlocked = true;
+                newFocusedElement = 0;
+            }
 
-        oldFocusedElement->dispatchFocusOutEvent(EventTypeNames::focusout,
newFocusedElement.get()); // DOM level 3 name for the bubbling blur event.
-        // FIXME: We should remove firing DOMFocusOutEvent event when we are
sure no content depends
-        // on it, probably when <rdar://problem/8503958> is resolved.
-        oldFocusedElement->dispatchFocusOutEvent(EventTypeNames::DOMFocusOut,
newFocusedElement.get()); // DOM level 2 name for compatibility.
+            oldFocusedElement->dispatchFocusOutEvent(EventTypeNames::focusout,
newFocusedElement.get()); // DOM level 3 name for the bubbling blur event.
+            // FIXME: We should remove firing DOMFocusOutEvent event when we
are sure no content depends
+            // on it, probably when <rdar://problem/8503958> is resolved.
+           
oldFocusedElement->dispatchFocusOutEvent(EventTypeNames::DOMFocusOut,
newFocusedElement.get()); // DOM level 2 name for compatibility.
 
-        if (m_focusedElement) {
-            // handler shifted focus
-            focusChangeBlocked = true;
-            newFocusedElement = 0;
+            if (m_focusedElement) {
+                // handler shifted focus
+                focusChangeBlocked = true;
+                newFocusedElement = 0;
+            }
         }
 
         if (view()) {
@@ -3332,31 +3336,35 @@ bool Document::setFocusedElement(PassRefPtr<Element>
prpNewFocusedElement, Focus
         m_focusedElement = newFocusedElement;
 
         // Dispatch the focus event and let the node do any other focus related
activities (important for text fields)
-        m_focusedElement->dispatchFocusEvent(oldFocusedElement.get(),
direction);
-
-        if (m_focusedElement != newFocusedElement) {
-            // handler shifted focus
-            focusChangeBlocked = true;
-            goto SetFocusedElementDone;
-        }
+        // If page lost focus, event will be dispatched on page focus, don't
duplicate
+        if (page() && (page()->focusController().isFocused() ||
!page()->focusController().beenFocused())) {
+            m_focusedElement->dispatchFocusEvent(oldFocusedElement.get(),
direction);
+
+            if (m_focusedElement != newFocusedElement) {
+                // handler shifted focus
+                focusChangeBlocked = true;
+                goto SetFocusedElementDone;
+            }
 
-        m_focusedElement->dispatchFocusInEvent(EventTypeNames::focusin,
oldFocusedElement.get()); // DOM level 3 bubbling focus event.
+            m_focusedElement->dispatchFocusInEvent(EventTypeNames::focusin,
oldFocusedElement.get()); // DOM level 3 bubbling focus event.
 
-        if (m_focusedElement != newFocusedElement) {
-            // handler shifted focus
-            focusChangeBlocked = true;
-            goto SetFocusedElementDone;
-        }
+            if (m_focusedElement != newFocusedElement) {
+                // handler shifted focus
+                focusChangeBlocked = true;
+                goto SetFocusedElementDone;
+            }
 
-        // FIXME: We should remove firing DOMFocusInEvent event when we are
sure no content depends
-        // on it, probably when <rdar://problem/8503958> is m.
-        m_focusedElement->dispatchFocusInEvent(EventTypeNames::DOMFocusIn,
oldFocusedElement.get()); // DOM level 2 for compatibility.
+            // FIXME: We should remove firing DOMFocusInEvent event when we are
sure no content depends
+            // on it, probably when <rdar://problem/8503958> is m.
+            m_focusedElement->dispatchFocusInEvent(EventTypeNames::DOMFocusIn,
oldFocusedElement.get()); // DOM level 2 for compatibility.
 
-        if (m_focusedElement != newFocusedElement) {
-            // handler shifted focus
-            focusChangeBlocked = true;
-            goto SetFocusedElementDone;
+            if (m_focusedElement != newFocusedElement) {
+                // handler shifted focus
+                focusChangeBlocked = true;
+                goto SetFocusedElementDone;
+            }
         }
+
         m_focusedElement->setFocus(true);
 
         if (m_focusedElement->isRootEditableElement())

[ljs, 2013-10-11 07:54:48]

PTAL.

Presumably this was due to a merge issue, I have rebased.

[commit-bot: I haz the power, 2013-10-11 08:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/50001

[commit-bot: I haz the power, 2013-10-11 10:59:12]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-11 11:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/50001

[commit-bot: I haz the power, 2013-10-11 12:42:50]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[ljs, 2013-10-11 12:55:42]

On 2013/10/11 12:42:50, I haz the power (commit-bot) wrote:
> Retried try job too often on win_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

Bizarre; it appears to have failed with tests that didn't fail last try + are
seem unrelated to the patch.

virtual/gpu/compositedscrolling/overflow/content-loses-scrollbars.html
virtual/gpu/compositedscrolling/overflow/nested-render-surfaces-with-intervening-clip.html
virtual/gpu/compositedscrolling/overflow/nested-render-surfaces-with-rotation.html

[ljs, 2013-10-11 20:22:15]

On 2013/10/11 12:55:42, ljs wrote:
> On 2013/10/11 12:42:50, I haz the power (commit-bot) wrote:
> > Retried try job too often on win_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
> 
> Bizarre; it appears to have failed with tests that didn't fail last try + are
> seem unrelated to the patch.
> 
> virtual/gpu/compositedscrolling/overflow/content-loses-scrollbars.html
>
virtual/gpu/compositedscrolling/overflow/nested-render-surfaces-with-intervening-clip.html
>
virtual/gpu/compositedscrolling/overflow/nested-render-surfaces-with-rotation.html

Would it be possible to send the patch to the try servers again? As I wonder
whether this is simply flakiness. The tests that fail vary (as far as can tell
from the stdio output) between trybot runs. I tried selecting win_blink_rel for
patch set 8, but I believe I lack permission to do that so it marked it as
failed, though it hasn't :)

[commit-bot: I haz the power, 2013-10-11 23:31:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/68001

[commit-bot: I haz the power, 2013-10-12 06:57:35]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-12 07:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/68001

[commit-bot: I haz the power, 2013-10-12 10:43:32]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[ljs, 2013-10-12 10:51:35]

On 2013/10/12 10:43:32, I haz the power (commit-bot) wrote:
> Retried try job too often on win_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

Ok, looks like we have a genuine failure in
inspector-protocol/dom/dom-focus.html on windows. This layout test passes
locally on my mac (and the try server suggests it passes on linux also), so I
will establish a windows development environment and investigate this.

[ljs, 2013-10-14 22:12:51]

PTAL.

I added code to track when the events are actually dispatched; because if they
aren't we shouldn't prevent event dispatch as we aren't then preventing
duplication. This appears to fix the failing test in windows, doesn't break any
of the unit tests + doesn't cause failures in any of the previously failing
layout tests.

[ljs, 2013-10-15 07:50:43]

On 2013/10/14 22:12:51, ljs wrote:
> PTAL.
> 
> I added code to track when the events are actually dispatched; because if they
> aren't we shouldn't prevent event dispatch as we aren't then preventing
> duplication. This appears to fix the failing test in windows, doesn't break
any
> of the unit tests + doesn't cause failures in any of the previously failing
> layout tests.

PTAL once more :)

I realised I needed to add an additional check such that focus events only fire
if the blur event has fired in dispatchEventsOnWindowAndFocusedNode, otherwise
it's feasible we could get a duplicated focus event.

This change as before passes all unit tests, previously failing layout tests,
and the test failing on windows.

[ljs, 2013-10-15 19:13:22]

Could somebody with the appropriate permissions submit this to the trybots? :)
Thanks!

[commit-bot: I haz the power, 2013-10-16 17:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/100001

[commit-bot: I haz the power, 2013-10-16 20:36:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/100001

[commit-bot: I haz the power, 2013-10-16 21:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/100001

[tkent, 2013-10-16 21:41:36]

not lgtm

canceling a stale lgtm.  I need to take a look at this again when I have enough
time.

[ljs, 2013-10-16 21:45:12]

On 2013/10/16 21:41:36, tkent wrote:
> not lgtm
> 
> canceling a stale lgtm.  I need to take a look at this again when I have
enough
> time.

Ok, apologies, I didn't know where we were with this + still getting used to the
whole system :) Let me know of issues when you get a chance + I will take a look
ASAP.

[tkent, 2013-10-18 00:00:45]

On 2013/10/14 22:12:51, ljs wrote:
> PTAL.
> 
> I added code to track when the events are actually dispatched; because if they
> aren't we shouldn't prevent event dispatch as we aren't then preventing
> duplication. This appears to fix the failing test in windows, doesn't break
any
> of the unit tests + doesn't cause failures in any of the previously failing
> layout tests.

Hmm, the new behavior looks strange.
1. A window has no focused element.
2. The window is deactivated.
3. An element is focused programmatically
Then, focus events are dispatched though the window is inactive.
Don't IE and firefox work so, right?

If dom-focus.html fails because the Inspector window is active, I think it's ok
to update the test.  For example, activate the main window before sending
DOM.focus,.

[ljs, 2013-10-20 15:26:37]

On 2013/10/18 00:00:45, tkent wrote:
> On 2013/10/14 22:12:51, ljs wrote:
> > PTAL.
> > 
> > I added code to track when the events are actually dispatched; because if
they
> > aren't we shouldn't prevent event dispatch as we aren't then preventing
> > duplication. This appears to fix the failing test in windows, doesn't break
> any
> > of the unit tests + doesn't cause failures in any of the previously failing
> > layout tests.
> 
> Hmm, the new behavior looks strange.
> 1. A window has no focused element.
> 2. The window is deactivated.
> 3. An element is focused programmatically
> Then, focus events are dispatched though the window is inactive.

Indeed! I missed that, and it's obviously a problem.

> Don't IE and firefox work so, right?

Indeed,  testing confirms unsurprisingly :) that IE and Firefox don't dispatch
any blur/focus events when the window is inactive, regardless of the currently
focused element.

> 
> If dom-focus.html fails because the Inspector window is active, I think it's
ok
> to update the test.  For example, activate the main window before sending
> DOM.focus,.

Done.

I have reverted the approach to the original one taken and have removed the
beenFocused() flag, which is unnecessary if the content shell actually actives +
focuses the webdev tools when they're shown, I have submitted a separate patch
for this over at https://codereview.chromium.org/31043002/.

I have run layout tests, content + webkit unit tests, tested the previously
failing tests mentioned in this thread, and tested the windows-only failure on
inspector-protocol/dom/dom-focus.html on my freshly set up windows chromium dev
environment (phew!) and each pass, assuming the aforementioned patch is present.

If for whatever reason that patch can't be applied, then we can revert to using
the beenFocused() flag and permit focus/blur events to be dispatched if a window
has never been made active, however this raises the case of:-

* Programmatically focus element on never-active page, focus event fires
* Activate page, focus event duplicates.

Which is simply wrong.

[ljs, 2013-10-20 21:21:09]

On 2013/10/20 15:26:37, ljs wrote:

> I have reverted the approach to the original one taken and have removed the
> beenFocused() flag, which is unnecessary if the content shell actually actives
+
> focuses the webdev tools when they're shown, I have submitted a separate patch
> for this over at https://codereview.chromium.org/31043002/.
> 
> ...
> 
> If for whatever reason that patch can't be applied...

This patch has now been committed.

[ljs, 2013-10-20 22:32:59]

Some of those trybots are tryin' revisions prior to the pre-requisite commit's
revision :) - it went in on 229709, and mac_layout, win_layout and linux_layout
are using 229707.

Also the failure on
virtual/gpu/fast/canvas/webgl/texture-transparent-pixels-initialized.html
appears to be an unrelated flake, have tested on both mac and windows and they
both pass, and I can't imagine the patch would have affected this.

[ljs, 2013-10-20 22:55:49]

On 2013/10/20 22:32:59, ljs wrote:
> Some of those trybots are tryin' revisions prior to the pre-requisite commit's
> revision :) - it went in on 229709, and mac_layout, win_layout and
linux_layout
> are using 229707.
> 
> Also the failure on
> virtual/gpu/fast/canvas/webgl/texture-transparent-pixels-initialized.html
> appears to be an unrelated flake, have tested on both mac and windows and they
> both pass, and I can't imagine the patch would have affected this.

And the other tests that failed seem spurious too:-

http/tests/inspector/network/network-xhr-async-response-type-blob.html
inspector/elements/iframe-load-event.html
media/event-attributes.html
media/media-fragments/TC0011.html
media/media-fragments/TC0054.html
scrollbars/scrollbar-drag-thumb-with-large-content.html

Each of these runs on mac (my dev machine), windows (my trusty windows dev
build), so I think we're just experiencing some flake here :)

[tkent, 2013-10-21 00:08:39]

lgtm

[commit-bot: I haz the power, 2013-10-21 00:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lstoakes@gmail.com/26149002/149001

[commit-bot: I haz the power, 2013-10-21 01:15:07]

Change committed as 160036

[tkent, 2013-10-21 01:18:36]

On 2013/10/21 01:15:07, I haz the power (commit-bot) wrote:
> Change committed as 160036

Thank you for the hard work!

[ljs, 2013-10-22 09:04:58]

This has been reverted due to flakiness caused in
BrowserFocusTest.FocusTraversal on XP, I will investigate and patch ASAP.