Repaint selection when element with ::selection style is recalculated.

Repaint selection when element with ::selection style is recalculated.

Selection was not repainted unless the selected text was repainted due
to other style changes. Now, if the ComputedStyle is recalculated for
an element and either the old or the new ComputedStyle had a bit set
for PseudoIdSelection, schedule paint invalidation for the selection
leaf children of that element.

Note that we don't need to traverse down the descendants because the
current implementation of ::selection in Blink only affects direct
children. The selection state is only propagated to containing block
ancestor, which is why we look for a containing block to check if any
of the children is selected.

R=mstensho@opera.com
BUG=685174
Review-Url: https://codereview.chromium.org/2709693003
Cr-Commit-Position: refs/heads/master@{#451992}
Committed: https://chromium.googlesource.com/chromium/src/+/eff357ef3a21515a5bfaa487b687d7d7353024f2

[rune, 2017-02-21 12:22:34]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[rune, 2017-02-21 12:22:41]

ptal

[commit-bot: I haz the power, 2017-02-21 12:22:49]

Dry run: 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-02-21 13:34:11]

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

[commit-bot: I haz the power, 2017-02-21 13:34:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mstensho (USE GERRIT), 2017-02-21 21:03:54]

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html
(right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html:12:
<span id="t1">GREEN<span> RED </span>GREEN</span>
The fact that "RED" is indeed painted in red here is an implementation-specific
thing, right (according to the commit comment)? So that if someone decides to
fix that, this test would break?

Is it really necessary to depend on this kind of shortcoming in Blink to test
your fix? I checked, and without your code changes, the test also seems to fail
with just an empty <span> between the two GREEN words.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3474: isLayoutBlock() ?
toLayoutBlock(this) : containingBlock();
Even if you also explain it in the commit comment, this deserves some
documentation right here, since it's kind of exotic.

So, LayoutBoxModelObject::setSelectionState() sets an accumulated state (start +
end = both) on itself and the containing block chain all the way to LayoutView,
and the code here is a consequence of that, right?

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3479: for (LayoutObject*
child = slowFirstChild(); child;
Likewise, I think it's worth mentioning right here why we don't bother to go
deeper than the direct children.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleDifference.h (right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleDifference.h:63:
DCHECK(!needsPaintInvalidation());
So, it looks like this one is failing in quite a few tests.

[rune, 2017-02-21 21:42:00]

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html
(right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html:12:
<span id="t1">GREEN<span> RED </span>GREEN</span>
On 2017/02/21 21:03:54, mstensho wrote:
> The fact that "RED" is indeed painted in red here is an
implementation-specific
> thing, right (according to the commit comment)? So that if someone decides to
> fix that, this test would break?

Yes, it was to prove to the reader of the test and code that the shallow layout
tree walk for leaves was enough, but it would also catch the inadequacy of this
code if the spec ends up differently. No-one's been able to specify this yet.
::selection was in a Selectors Level 3 CR, under-specified, but was removed
later. Also see https://drafts.csswg.org/css-pseudo-4/#highlight-cascade

> Is it really necessary to depend on this kind of shortcoming in Blink to test
> your fix? I checked, and without your code changes, the test also seems to
fail
> with just an empty <span> between the two GREEN words.

No. I can simplify it.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3474: isLayoutBlock() ?
toLayoutBlock(this) : containingBlock();
On 2017/02/21 21:03:54, mstensho wrote:
> Even if you also explain it in the commit comment, this deserves some
> documentation right here, since it's kind of exotic.

OK.

> So, LayoutBoxModelObject::setSelectionState() sets an accumulated state (start
+
> end = both) on itself and the containing block chain all the way to
LayoutView,
> and the code here is a consequence of that, right?

Yes.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3479: for (LayoutObject*
child = slowFirstChild(); child;
On 2017/02/21 21:03:54, mstensho wrote:
> Likewise, I think it's worth mentioning right here why we don't bother to go
> deeper than the direct children.

Acknowledged.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleDifference.h (right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleDifference.h:63:
DCHECK(!needsPaintInvalidation());
On 2017/02/21 21:03:54, mstensho wrote:
> So, it looks like this one is failing in quite a few tests.

Yes. This is fragile since it relies on order and logic in
visualInvalidationDiff(). I've mad this an if-test in PS2 instead.

[rune, 2017-02-21 22:34:57]

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html
(right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/invalidation/selection/selection-background-repaint.html:12:
<span id="t1">GREEN<span> RED </span>GREEN</span>
On 2017/02/21 21:42:00, rune wrote:
> On 2017/02/21 21:03:54, mstensho wrote:
> > The fact that "RED" is indeed painted in red here is an
> implementation-specific
> > thing, right (according to the commit comment)? So that if someone decides
to
> > fix that, this test would break?
> 
> Yes, it was to prove to the reader of the test and code that the shallow
layout
> tree walk for leaves was enough, but it would also catch the inadequacy of
this
> code if the spec ends up differently. No-one's been able to specify this yet.
> ::selection was in a Selectors Level 3 CR, under-specified, but was removed
> later. Also see https://drafts.csswg.org/css-pseudo-4/#highlight-cascade
> 
> > Is it really necessary to depend on this kind of shortcoming in Blink to
test
> > your fix? I checked, and without your code changes, the test also seems to
> fail
> > with just an empty <span> between the two GREEN words.
> 
> No. I can simplify it.

Done.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3474: isLayoutBlock() ?
toLayoutBlock(this) : containingBlock();
On 2017/02/21 21:42:00, rune wrote:
> On 2017/02/21 21:03:54, mstensho wrote:
> > Even if you also explain it in the commit comment, this deserves some
> > documentation right here, since it's kind of exotic.
> 
> OK.
> 
> > So, LayoutBoxModelObject::setSelectionState() sets an accumulated state
(start
> +
> > end = both) on itself and the containing block chain all the way to
> LayoutView,
> > and the code here is a consequence of that, right?
> 
> Yes.

Done.

https://codereview.chromium.org/2709693003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3479: for (LayoutObject*
child = slowFirstChild(); child;
On 2017/02/21 21:42:00, rune wrote:
> On 2017/02/21 21:03:54, mstensho wrote:
> > Likewise, I think it's worth mentioning right here why we don't bother to go
> > deeper than the direct children.
> 
> Acknowledged.

Done.

[rune, 2017-02-21 22:39:17]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 22:40:47]

Dry run: 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-02-22 00:44:56]

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

[commit-bot: I haz the power, 2017-02-22 00:44:57]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[mstensho (USE GERRIT), 2017-02-22 09:49:58]

lgtm

[rune, 2017-02-22 09:50:31]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-02-22 09:51:26]

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-02-22 11:13:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487757031681200,
"parent_rev": "0f198df6bc8da6fe51b34f11b50936700b6e9353", "commit_rev":
"eff357ef3a21515a5bfaa487b687d7d7353024f2"}

[commit-bot: I haz the power, 2017-02-22 11:14:34]

Description was changed from

==========
Repaint selection when element with ::selection style is recalculated.

Selection was not repainted unless the selected text was repainted due
to other style changes. Now, if the ComputedStyle is recalculated for
an element and either the old or the new ComputedStyle had a bit set
for PseudoIdSelection, schedule paint invalidation for the selection
leaf children of that element.

Note that we don't need to traverse down the descendants because the
current implementation of ::selection in Blink only affects direct
children. The selection state is only propagated to containing block
ancestor, which is why we look for a containing block to check if any
of the children is selected.

R=mstensho@opera.com
BUG=685174
==========

to

==========
Repaint selection when element with ::selection style is recalculated.

Selection was not repainted unless the selected text was repainted due
to other style changes. Now, if the ComputedStyle is recalculated for
an element and either the old or the new ComputedStyle had a bit set
for PseudoIdSelection, schedule paint invalidation for the selection
leaf children of that element.

Note that we don't need to traverse down the descendants because the
current implementation of ::selection in Blink only affects direct
children. The selection state is only propagated to containing block
ancestor, which is why we look for a containing block to check if any
of the children is selected.

R=mstensho@opera.com
BUG=685174

Review-Url: https://codereview.chromium.org/2709693003
Cr-Commit-Position: refs/heads/master@{#451992}
Committed:
https://chromium.googlesource.com/chromium/src/+/eff357ef3a21515a5bfaa487b687...
==========

[commit-bot: I haz the power, 2017-02-22 11:14:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/eff357ef3a21515a5bfaa487b687...