Avoid unnecessary style recalc for subtree of focused element.

Avoid unnecessary style recalc for subtree of focused element.

Updating the styles for an element that changes its focus state only needs to
force a recalc of the subtree if the subtree contains an element that could
match a selector which contains a :focus pseudo class left of a child or
descendant combinator. At the same time, that :focus pseudo must match the
element we are updating focus state for. This is indicated by the return value
of Element::childrenAffectedByFocus.

This change does a LocalStyleChange when updating the focus state of an
element which returns false for childrenAffectedByFocus, and
SubtreeStyleChange otherwise.

BUG=315923
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166517

[esprehn, 2014-01-22 20:58:52]

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:794: static inline StyleChangeType
focusStyleChange(const ContainerNode* node)
This should be a private method, you always pass |this|.

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:799: if (node->isElementNode() &&
(toElement(node)->childrenAffectedByFocus() || toElement(node)->shadow()))
This shadow check doesn't seem right, why do you need to do a subtree recalc if
the thing has a shadow?

[rune, 2014-01-22 21:58:49]

Thanks for reviewing. I've found issues with generated content (before/after)
too. I'm digging my way through various issues with LocalStyleChange. I think
this needs to be on hold until I've fixed some other issues.

[rune, 2014-01-30 17:24:40]

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:799: if (node->isElementNode() &&
(toElement(node)->childrenAffectedByFocus() || toElement(node)->shadow()))
On 2014/01/22 20:58:52, esprehn wrote:
> This shadow check doesn't seem right, why do you need to do a subtree recalc
if
> the thing has a shadow?

It's about the propagation of -webkit-user-modify for contenteditable into
distributed nodes. I've split out it out as a separate issue[1]/CL with a
different solution[2] and will remove the special casing here.

[1] https://code.google.com/p/chromium/issues/detail?id=339463
[2] https://codereview.chromium.org/144853004/

[rune, 2014-01-30 23:24:36]

This CL is now ready for review again.

Since the shadow dom issue has been split out, it relies on the fix for issue
339463 to land.

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:794: static inline StyleChangeType
focusStyleChange(const ContainerNode* node)
On 2014/01/22 20:58:52, esprehn wrote:
> This should be a private method, you always pass |this|.

Done. That is, the method is now gone.

https://codereview.chromium.org/135183002/diff/150001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:799: if (node->isElementNode() &&
(toElement(node)->childrenAffectedByFocus() || toElement(node)->shadow()))
On 2014/01/30 17:24:40, rune - CET wrote:
> On 2014/01/22 20:58:52, esprehn wrote:
> > This shadow check doesn't seem right, why do you need to do a subtree recalc
> if
> > the thing has a shadow?
> 
> It's about the propagation of -webkit-user-modify for contenteditable into
> distributed nodes. I've split out it out as a separate issue[1]/CL with a
> different solution[2] and will remove the special casing here.
> 
> [1] https://code.google.com/p/chromium/issues/detail?id=339463
> [2] https://codereview.chromium.org/144853004/

Done.

[eseidel, 2014-01-31 06:20:33]

Its so awesome that you're working on this, thank you.  This causes jank on
first click to every site with an animation. :)

[eseidel, 2014-01-31 06:25:18]

As cool as it is to see more unittest infrastructure built up, I wonder if this
wouldn't be easier to write as a layout test.  I believe we expose information
about how many elements had their style recalcualted from window.internals.

In any case, looks good.

https://codereview.chromium.org/135183002/diff/330001/Source/core/css/Affecte...
File Source/core/css/AffectedByFocusTest.cpp (right):

https://codereview.chromium.org/135183002/diff/330001/Source/core/css/Affecte...
Source/core/css/AffectedByFocusTest.cpp:20: class AffectedByFocusTest : public
::testing::Test {
Very cool that we're building up more unittest architecture for this sort of
thing.

https://codereview.chromium.org/135183002/diff/330001/Source/core/css/Affecte...
Source/core/css/AffectedByFocusTest.cpp:72: // A global :focus rule in html.css
currently causes every single element to be
We have a bug open with the w3c about this: 
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23854

[eseidel, 2014-01-31 06:25:46]

lgtm

[rune, 2014-01-31 13:26:42]

On 2014/01/31 06:25:18, eseidel wrote:
> As cool as it is to see more unittest infrastructure built up, I wonder if
this
> wouldn't be easier to write as a layout test.  I believe we expose information
> about how many elements had their style recalcualted from window.internals.

I did use the timeline first, but that has a lot of overhead and I was asked not
to use it for other testing than timeline testing.

updateStyleAndReturnAffectedElementCount() is exposed on internals, but the
styles are already updated when focus() returns. An alternative would be to
expose resolverAccessCount() on internals directly and compare before and after
values.

[rune, 2014-02-04 16:44:24]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-04 16:44:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/330001

[commit-bot: I haz the power, 2014-02-04 16:44:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 16:44:34]

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

Patch:       Source/core/dom/ContainerNode.cpp
Index: Source/core/dom/ContainerNode.cpp
diff --git a/Source/core/dom/ContainerNode.cpp
b/Source/core/dom/ContainerNode.cpp
index
2f162a0cb0305a279256f431cf7689ece6d524c5..bb6f72bfbdd85ebd4372a2e140aca270f2c4c164
100644
--- a/Source/core/dom/ContainerNode.cpp
+++ b/Source/core/dom/ContainerNode.cpp
@@ -805,10 +805,12 @@ void ContainerNode::focusStateChanged()
     // renderer we can just ignore the state change.
     if (!renderer())
         return;
-    // FIXME: This could probably setNeedsStyleRecalc(LocalStyleChange) in the
affectedByFocus case
-    // and only setNeedsStyleRecalc(SubtreeStyleChange) in the
childrenAffectedByFocus case.
-    if (renderStyle()->affectedByFocus() || (isElementNode() &&
toElement(this)->childrenAffectedByFocus()))
+
+    if (isElementNode() && toElement(this)->childrenAffectedByFocus())
         setNeedsStyleRecalc();
+    else if (renderStyle()->affectedByFocus())
+        setNeedsStyleRecalc(LocalStyleChange);
+
     if (renderer() && renderer()->style()->hasAppearance())
         RenderTheme::theme().stateChanged(renderer(), FocusState);
 }
@@ -821,9 +823,15 @@ void ContainerNode::setFocus(bool received)
     Node::setFocus(received);
 
     focusStateChanged();
+
+    if (renderer() || received)
+        return;
+
     // If :focus sets display: none, we lose focus but still need to recalc our
style.
-    if (!renderer() && !received)
+    if (isElementNode() && toElement(this)->childrenAffectedByFocus())
         setNeedsStyleRecalc();
+    else
+        setNeedsStyleRecalc(LocalStyleChange);
 }
 
 void ContainerNode::setActive(bool down)

[rune, 2014-02-04 17:30:53]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-04 17:31:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/400001

[commit-bot: I haz the power, 2014-02-04 18:38:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 18:38:13]

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

[rune, 2014-02-04 19:05:24]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-04 19:05:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/630001

[commit-bot: I haz the power, 2014-02-04 22:26:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:26:58]

Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[rune, 2014-02-05 10:06:54]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-05 10:07:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/630001

[commit-bot: I haz the power, 2014-02-05 10:46:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 10:46:40]

Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[rune, 2014-02-05 12:44:30]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-05 12:44:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/860001

[commit-bot: I haz the power, 2014-02-05 14:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 14:12:00]

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

[rune, 2014-02-05 14:25:04]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-05 14:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/860001

[commit-bot: I haz the power, 2014-02-05 15:56:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 15:56:01]

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

[rune, 2014-02-05 17:09:03]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-02-05 17:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/135183002/860001

[commit-bot: I haz the power, 2014-02-05 17:32:37]

Change committed as 166517