[selectors4] Use common ancestor strategy for :focus-within

[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
verifying that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html

Review-Url: https://codereview.chromium.org/2821303005
Cr-Commit-Position: refs/heads/master@{#466949}
Committed: https://chromium.googlesource.com/chromium/src/+/8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2

[Manuel Rego, 2017-04-19 09:53:25]

rego@igalia.com changed reviewers:
+ rune@opera.com

[Manuel Rego, 2017-04-19 09:53:26]

As suggested in the initial review for adding :focus-within support [1],
this patch uses the common ancestor strategy similar to :active and :hover
for :focus-within.

[1] https://codereview.chromium.org/2795143004/

[rune, 2017-04-20 09:22:17]

https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1055:
OwnerShadowHost()->SetFocused(received, focus_type);
Having a default argument for common_ancestor looks scary to me. Here we pass
nullptr as the ancestor, right?

Wouldn't it be better if we updated the :focus-within state from
Document::SetFocusedElement()?

Is the problem that focus, hence focus-within, state can change without the
focus element being changed because the whole window loses focus?

[Manuel Rego, 2017-04-20 15:20:37]

Thanks for the review.

https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2821303005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1055:
OwnerShadowHost()->SetFocused(received, focus_type);
On 2017/04/20 09:22:17, rune wrote:
> Having a default argument for common_ancestor looks scary to me. Here we pass
> nullptr as the ancestor, right?

Yeah, this is an oversight in this version of the patch,
as I was trying the other way around before.

> Wouldn't it be better if we updated the :focus-within state from
> Document::SetFocusedElement()?
> 
> Is the problem that focus, hence focus-within, state can change without the
> focus element being changed because the whole window loses focus?

That was my original idea, but I found that strange problem
when the whole window lose focus. [1]
And I thought there might be more cases where ContainerNode::SetFocused()
was called directly bypassing Document::SetFocusedElement().

But checking the result of this first patch I agree with you that
it's not looking good. :(

I'm uploading a new version, modifying Document::SetFocusedElement(),
which also needs some small changes in FocusController.
Let's see what you think about that.

Of course, any other idea is welcomed.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Focu...

[rune, 2017-04-21 08:08:35]

Great! This looks a lot better to me.

Looking at this yesterday, the changes where a whole frame/window loses/gains
focus is where we don't hit the Document::SetFocusedElement() path as the
document.activeElement is not reset on such focus changes.

If the focus controller changes cover everything this looks good.

Could you add some tests for this? I was thinking particularly about
:focus-within tests in fast/css/invalidation and the focus/blur of a frame
covered by the FocusController change.

https://codereview.chromium.org/2821303005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2821303005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:375:
focused_element->SetHasFocusWithinUpToAncestor(false);
I'm generally not so fond of default arguments and would prefer an explicit null
ancestor. Reading this code, I wonder which ancestor is that method referring
to?

[Manuel Rego, 2017-04-24 15:24:46]

On 2017/04/21 08:08:35, rune wrote:
> Looking at this yesterday, the changes where a whole frame/window loses/gains
> focus is where we don't hit the Document::SetFocusedElement() path as the
> document.activeElement is not reset on such focus changes.
> 
> If the focus controller changes cover everything this looks good.

Yeah that's the only thing I found so far.

> Could you add some tests for this? I was thinking particularly about
> :focus-within tests in fast/css/invalidation and the focus/blur of a frame
> covered by the FocusController change.

For the FocusController change we're covered by:
fast/selectors/focus-within-window-inactive.html

I've been trying to create a fast/css/invalidation without success. :-(
This is my try: http://sprunge.us/IISV?html
And the output is:
CONSOLE MESSAGE: line 18: Initial:
	* updateStyleAndReturnAffectedElementCount: 7
	* color: rgb(0, 0, 0)
	* background-color: rgb(255, 255, 255)
CONSOLE MESSAGE: line 24: After layout:
	* updateStyleAndReturnAffectedElementCount: 0
	* color: rgb(0, 0, 0)
	* background-color: rgb(255, 255, 255)
CONSOLE MESSAGE: line 30: After focus():
	* updateStyleAndReturnAffectedElementCount: 0
	* color: rgb(0, 255, 0)
	* background-color: rgb(0, 0, 255)
CONSOLE MESSAGE: line 36: Again after layout:
	* updateStyleAndReturnAffectedElementCount: 0
	* color: rgb(0, 255, 0)
	* background-color: rgb(0, 0, 255)

I don't know how I can check that the focus() call modifies the styles.
Any idea?

https://codereview.chromium.org/2821303005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2821303005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:375:
focused_element->SetHasFocusWithinUpToAncestor(false);
On 2017/04/21 08:08:35, rune wrote:
> I'm generally not so fond of default arguments and would prefer an explicit
null
> ancestor. Reading this code, I wonder which ancestor is that method referring
> to?

Ok, I've made it mandatory.

[Manuel Rego, 2017-04-24 15:29:08]

Description was changed from

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to ContainerNode::SetFocused().
Now ContainerNode::SetFocused() stops to set :focus-within flag when
it gets the given common ancestor.

BUG=617371
==========

to

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

BUG=617371
==========

[rune, 2017-04-25 06:51:04]

On 2017/04/24 15:24:46, Manuel Rego wrote:
> On 2017/04/21 08:08:35, rune wrote:

> > Could you add some tests for this? I was thinking particularly about
> > :focus-within tests in fast/css/invalidation and the focus/blur of a frame
> > covered by the FocusController change.
> 
> For the FocusController change we're covered by:
> fast/selectors/focus-within-window-inactive.html

Could you add that to TEST= in the description?

> I've been trying to create a fast/css/invalidation without success. :-(

> This is my try: http://sprunge.us/IISV?html
> And the output is:
> CONSOLE MESSAGE: line 36: Again after layout:
> 	* updateStyleAndReturnAffectedElementCount: 0
> 	* color: rgb(0, 255, 0)
> 	* background-color: rgb(0, 0, 255)
> 
> I don't know how I can check that the focus() call modifies the styles.
> Any idea?

Right, it's because focus() updates style synchronously, so
updateStyleAndReturnAffected... will give you 0.

You can do unit tests in AffectedByFocusTest.cpp instead.

[Manuel Rego, 2017-04-25 08:09:34]

Description was changed from

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

BUG=617371
==========

to

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
veryfing that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html
==========

[Manuel Rego, 2017-04-25 08:09:52]

Description was changed from

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
veryfing that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html
==========

to

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
verifying that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html
==========

[Manuel Rego, 2017-04-25 08:15:26]

On 2017/04/25 06:51:04, rune wrote:
> On 2017/04/24 15:24:46, Manuel Rego wrote:
> > On 2017/04/21 08:08:35, rune wrote:
> 
> > > Could you add some tests for this? I was thinking particularly about
> > > :focus-within tests in fast/css/invalidation and the focus/blur of a frame
> > > covered by the FocusController change.
> > 
> > For the FocusController change we're covered by:
> > fast/selectors/focus-within-window-inactive.html
> 
> Could you add that to TEST= in the description?

Done.

> > I've been trying to create a fast/css/invalidation without success. :-(
> 
> > This is my try: http://sprunge.us/IISV?html
> > And the output is:
> > CONSOLE MESSAGE: line 36: Again after layout:
> > 	* updateStyleAndReturnAffectedElementCount: 0
> > 	* color: rgb(0, 255, 0)
> > 	* background-color: rgb(0, 0, 255)
> > 
> > I don't know how I can check that the focus() call modifies the styles.
> > Any idea?
> 
> Right, it's because focus() updates style synchronously, so
> updateStyleAndReturnAffected... will give you 0.
> 
> You can do unit tests in AffectedByFocusTest.cpp instead.

Yeah, good idea, I was losing my time
fighting with updateStyleAndReturnAffectedElementCount(). Thanks!

Just uploaded a new version with the unit test.

[rune, 2017-04-25 09:27:33]

lgtm with nits.

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp (right):

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:338: ASSERT_EQ(3U,
element_count);
EXPECT_EQ should suffice.

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:350: ASSERT_EQ(2U,
element_count);
EXPECT_EQ.

[Manuel Rego, 2017-04-25 09:37:43]

Thanks for the review!

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp (right):

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:338: ASSERT_EQ(3U,
element_count);
On 2017/04/25 09:27:33, rune wrote:
> EXPECT_EQ should suffice.

Acknowledged.

https://codereview.chromium.org/2821303005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/AffectedByFocusTest.cpp:350: ASSERT_EQ(2U,
element_count);
On 2017/04/25 09:27:33, rune wrote:
> EXPECT_EQ.

Acknowledged.

[Manuel Rego, 2017-04-25 09:38:02]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2017-04-25 09:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2821303005/#ps80001
(title: "Use EXPECT_EQ instead of ASSERT_EQ")

[commit-bot: I haz the power, 2017-04-25 09:38:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 10:44:13]

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

[commit-bot: I haz the power, 2017-04-25 10:44:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Manuel Rego, 2017-04-25 10:58:02]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2017-04-25 10:58:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 12:06:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493117882658150,
"parent_rev": "2aa1a170c5f8709ce1e1fde42cf49c76d4e24a9c", "commit_rev":
"8789e1ce7cc6848e0cb4ba4bb546828df4fe7ca2"}

[commit-bot: I haz the power, 2017-04-25 12:06:59]

Description was changed from

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
verifying that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html
==========

to

==========
[selectors4] Use common ancestor strategy for :focus-within

Following the same approach than what we do for :active and :hover
pseudo-classes, it seems a nice optimization to use the common ancestor
strategy for :focus-within too.

The use case would be for example that you've a from with several inputs
in a deep part of the DOM, and in the body element you've :focus-within
to show some warning while editing the form.
When you switch focus from one input to the next one, we don't need
to invalidate the whole chain up to the body, but it'd be enough
to just do it up to the common ancestor of the old and new focused elements
(in this example the form).

To achieve that Document::SetFocusedElement() has been modified to look
for the common ancestor and then pass it to
ContainerNode::SetHasFocusWithinUpToAncestor().
This new helper method is also used from FocusController to set
:focus-within flag when the window loses/gains focus.

A new unit test has been added to AffectedByFocusTest.cpp
verifying that we only recalculate styles up to the common ancestor.

BUG=617371
TEST=fast/selectors/focus-within-window-inactive.html

Review-Url: https://codereview.chromium.org/2821303005
Cr-Commit-Position: refs/heads/master@{#466949}
Committed:
https://chromium.googlesource.com/chromium/src/+/8789e1ce7cc6848e0cb4ba4bb546...
==========

[commit-bot: I haz the power, 2017-04-25 12:07:00]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8789e1ce7cc6848e0cb4ba4bb546...