Fix inline focus ring paint invalidation on continuation change

Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=704243, 703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2765763002
Cr-Commit-Position: refs/heads/master@{#458895}
Committed: https://chromium.googlesource.com/chromium/src/+/437d0b9a1fbd7345b234e54e09716f501c37ccb8

[Xianzhu, 2017-03-21 02:53:08]

Description was changed from

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=703403
==========

to

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-03-21 02:53:10]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 02:53:56]

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-03-21 04:12:46]

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

[commit-bot: I haz the power, 2017-03-21 04:12:47]

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

[Xianzhu, 2017-03-21 15:42:10]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[Xianzhu, 2017-03-21 17:50:49]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:51:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-03-21 18:40:49]

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3427: object =
object->paintInvalidationParent()) {
Do we need to invalidate both the object and its continuation start? In other
words, could paintInvalidationParent just return the continuation start in this
case?

[commit-bot: I haz the power, 2017-03-21 19:22:27]

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

[commit-bot: I haz the power, 2017-03-21 19:22:29]

Dry run: This issue passed the CQ dry run.

[wkorman, 2017-03-21 19:31:52]

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:148: crbug.com/703403
paint/inline/focus-ring-under-absolute-with-relative-continuation.html [ Failure
]
http://crbug.com/700523 looks fixed, try rebaseline-cl again?

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3430: // If the object is
an anonymous block continuation, mark the start object
Wondered why we only have to mark the start object. Presuming this is an
artifact of how we implement invalidation of continuations, or, the rest of the
continuation will be handled when the other involved objects are invalidated?
Perhaps worth adding to comment.

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockFlowPaintInvalidator.cpp (left):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockFlowPaintInvalidator.cpp:72:
.slowSetPaintingLayerNeedsRepaintAndInvalidateDisplayItemClient(
This change removes this display item client invalidation. Is that handled now
through the paint-offset-and-visual-rect bit processing logic?

[Xianzhu, 2017-03-22 00:55:29]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 00:56:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-03-22 02:46:08]

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:148: crbug.com/703403
paint/inline/focus-ring-under-absolute-with-relative-continuation.html [ Failure
]
On 2017/03/21 19:31:52, wkorman wrote:
> http://crbug.com/700523 looks fixed, try rebaseline-cl again?

No. I discovered this bug when I was working on this CL which won't fix it.

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3427: object =
object->paintInvalidationParent()) {
On 2017/03/21 18:40:49, pdr. wrote:
> Do we need to invalidate both the object and its continuation start? In other
> words, could paintInvalidationParent just return the continuation start in
this
> case?

This special case is for focus ring only. Focus ring is the only case that the
visual rect of the start of continuation may depends on continuations. All other
cases just use the normal hierarchy.

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3430: // If the object is
an anonymous block continuation, mark the start object
On 2017/03/21 19:31:52, wkorman wrote:
> Wondered why we only have to mark the start object. Presuming this is an
> artifact of how we implement invalidation of continuations, or, the rest of
the
> continuation will be handled when the other involved objects are invalidated?
> Perhaps worth adding to comment.

Please see the reply to pdr@'s comment above.

Updated comment to state that this is special case for focus ring only.

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockFlowPaintInvalidator.cpp (left):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockFlowPaintInvalidator.cpp:72:
.slowSetPaintingLayerNeedsRepaintAndInvalidateDisplayItemClient(
On 2017/03/21 19:31:52, wkorman wrote:
> This change removes this display item client invalidation. Is that handled now
> through the paint-offset-and-visual-rect bit processing logic?

Yes. With setMayNeedPaintInvalidation(), we will update visual rect and
invalidate the whole object with outline.

[commit-bot: I haz the power, 2017-03-22 02:51:11]

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

[commit-bot: I haz the power, 2017-03-22 02:51:12]

Dry run: 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_...)

[wkorman, 2017-03-22 18:10:56]

lgtm

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:148: crbug.com/703403
paint/inline/focus-ring-under-absolute-with-relative-continuation.html [ Failure
]
On 2017/03/22 02:46:07, Xianzhu wrote:
> On 2017/03/21 19:31:52, wkorman wrote:
> > http://crbug.com/700523 looks fixed, try rebaseline-cl again?
> 
> No. I discovered this bug when I was working on this CL which won't fix it. 

What is the new bug? Just curious so that I can track it. Or are you saying the
referenced one is still open?

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3432: // Mark the start
object for paint invalidation if it has focus ring.
My core question was actually, what ends up taking care of paint invalidation of
the rest of the continuation? I don't know much about continuations yet so am
interested to know for myself for future.

Or, put differently, what is special about the start, why do we not need similar
special code to mark mid- and end-continuations?

[Xianzhu, 2017-03-22 18:56:51]

Description was changed from

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=704243, 703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-03-22 19:18:13]

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:148: crbug.com/703403
paint/inline/focus-ring-under-absolute-with-relative-continuation.html [ Failure
]
On 2017/03/22 18:10:56, wkorman wrote:
> On 2017/03/22 02:46:07, Xianzhu wrote:
> > On 2017/03/21 19:31:52, wkorman wrote:
> > > http://crbug.com/700523 looks fixed, try rebaseline-cl again?
> > 
> > No. I discovered this bug when I was working on this CL which won't fix it. 
> 
> What is the new bug? Just curious so that I can track it. Or are you saying
the
> referenced one is still open?

The bug is not new but newly discovered during the work for this CL and this CL
doesn't fix it. The bug is referenced because this CL adds a test case for it,
and the existing test case affected by the bug is modified to avoid it and show
the real bug for which the test is supposed to test.

Filed crbug.com/704243 which is the bug that the CL is fixing.

Sorry for confusing.

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3432: // Mark the start
object for paint invalidation if it has focus ring.
On 2017/03/22 18:10:56, wkorman wrote:
> My core question was actually, what ends up taking care of paint invalidation
of
> the rest of the continuation? I don't know much about continuations yet so am
> interested to know for myself for future.
> 
> Or, put differently, what is special about the start, why do we not need
similar
> special code to mark mid- and end-continuations?

<span style="outline: auto">
  ABC
  <div style="position: relative; width: 100px; height: 100px"></div>
  DEF
</span>

The layout tree is like:

      LayoutBlockFlow (anonymous) 0x11073c21c4d0
        LayoutInline 0x11073c22c010 continuation=0x11073c21c3a0	SPAN
style="outline: auto"
          LayoutText 0x11073c22c0d8    	#text "\n  ABC\n  "
      LayoutBlockFlow (anonymous) 0x11073c21c3a0 continuation=0x11073c22c1a0
        LayoutBlockFlow (relative positioned) 0x11073c21c270	DIV
style="position: relative; width: 100px; height: 100px"
      LayoutBlockFlow (anonymous) 0x11073c21c600
        LayoutInline 0x11073c22c1a0    	SPAN style="outline: auto"
          LayoutText 0x11073c22c268    	#text "\n  DEF\n"
        LayoutText 0x11073c22c330      	#text "\n"

The span is the start object of the continuation. It needs to paint a focus ring
enclosing the div (I guess the behavior is simply because of historical
reasons), though the div doesn't belong to the span in the layout tree. Focus
ring is the only case that paint invalidation of start of continuation depends
on continuations. For any other cases, paint invalidation just happens normally
in the above layout tree.

We have special condition to prevent the tail of the continuation
(0x11073c22c1a0 in the above tree) from painting the outline again.

[pdr., 2017-03-22 20:05:17]

On 2017/03/22 at 19:18:13, wangxianzhu wrote:
>
https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2765763002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:148: crbug.com/703403
paint/inline/focus-ring-under-absolute-with-relative-continuation.html [ Failure
]
> On 2017/03/22 18:10:56, wkorman wrote:
> > On 2017/03/22 02:46:07, Xianzhu wrote:
> > > On 2017/03/21 19:31:52, wkorman wrote:
> > > > http://crbug.com/700523 looks fixed, try rebaseline-cl again?
> > > 
> > > No. I discovered this bug when I was working on this CL which won't fix
it. 
> > 
> > What is the new bug? Just curious so that I can track it. Or are you saying
the
> > referenced one is still open?
> 
> The bug is not new but newly discovered during the work for this CL and this
CL doesn't fix it. The bug is referenced because this CL adds a test case for
it, and the existing test case affected by the bug is modified to avoid it and
show the real bug for which the test is supposed to test.
> 
> Filed crbug.com/704243 which is the bug that the CL is fixing.
> 
> Sorry for confusing.
> 
>
https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:3432: // Mark the start
object for paint invalidation if it has focus ring.
> On 2017/03/22 18:10:56, wkorman wrote:
> > My core question was actually, what ends up taking care of paint
invalidation of
> > the rest of the continuation? I don't know much about continuations yet so
am
> > interested to know for myself for future.
> > 
> > Or, put differently, what is special about the start, why do we not need
similar
> > special code to mark mid- and end-continuations?
> 
> <span style="outline: auto">
>   ABC
>   <div style="position: relative; width: 100px; height: 100px"></div>
>   DEF
> </span>
> 
> The layout tree is like:
> 
>       LayoutBlockFlow (anonymous) 0x11073c21c4d0
>         LayoutInline 0x11073c22c010 continuation=0x11073c21c3a0	SPAN
style="outline: auto"
>           LayoutText 0x11073c22c0d8    	#text "\n  ABC\n  "
>       LayoutBlockFlow (anonymous) 0x11073c21c3a0 continuation=0x11073c22c1a0
>         LayoutBlockFlow (relative positioned) 0x11073c21c270	DIV
style="position: relative; width: 100px; height: 100px"
>       LayoutBlockFlow (anonymous) 0x11073c21c600
>         LayoutInline 0x11073c22c1a0    	SPAN style="outline: auto"
>           LayoutText 0x11073c22c268    	#text "\n  DEF\n"
>         LayoutText 0x11073c22c330      	#text "\n"
> 
> The span is the start object of the continuation. It needs to paint a focus
ring enclosing the div (I guess the behavior is simply because of historical
reasons), though the div doesn't belong to the span in the layout tree. Focus
ring is the only case that paint invalidation of start of continuation depends
on continuations. For any other cases, paint invalidation just happens normally
in the above layout tree.
> 
> We have special condition to prevent the tail of the continuation
(0x11073c22c1a0 in the above tree) from painting the outline again.

LGTM

[Xianzhu, 2017-03-22 20:06:22]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-03-22 20:06:58]

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

[wkorman, 2017-03-22 20:28:54]

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2765763002/diff/30025/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3432: // Mark the start
object for paint invalidation if it has focus ring.
On 2017/03/22 19:18:13, Xianzhu wrote:
> On 2017/03/22 18:10:56, wkorman wrote:
> > My core question was actually, what ends up taking care of paint
invalidation
> of
> > the rest of the continuation? I don't know much about continuations yet so
am
> > interested to know for myself for future.
> > 
> > Or, put differently, what is special about the start, why do we not need
> similar
> > special code to mark mid- and end-continuations?
> 
> <span style="outline: auto">
>   ABC
>   <div style="position: relative; width: 100px; height: 100px"></div>
>   DEF
> </span>
> 
> The layout tree is like:
> 
>       LayoutBlockFlow (anonymous) 0x11073c21c4d0
>         LayoutInline 0x11073c22c010 continuation=0x11073c21c3a0	SPAN
> style="outline: auto"
>           LayoutText 0x11073c22c0d8    	#text "\n  ABC\n  "
>       LayoutBlockFlow (anonymous) 0x11073c21c3a0 continuation=0x11073c22c1a0
>         LayoutBlockFlow (relative positioned) 0x11073c21c270	DIV
> style="position: relative; width: 100px; height: 100px"
>       LayoutBlockFlow (anonymous) 0x11073c21c600
>         LayoutInline 0x11073c22c1a0    	SPAN style="outline: auto"
>           LayoutText 0x11073c22c268    	#text "\n  DEF\n"
>         LayoutText 0x11073c22c330      	#text "\n"
> 
> The span is the start object of the continuation. It needs to paint a focus
ring
> enclosing the div (I guess the behavior is simply because of historical
> reasons), though the div doesn't belong to the span in the layout tree. Focus
> ring is the only case that paint invalidation of start of continuation depends
> on continuations. For any other cases, paint invalidation just happens
normally
> in the above layout tree.
> 
> We have special condition to prevent the tail of the continuation
> (0x11073c22c1a0 in the above tree) from painting the outline again.

Ah, I see. Thank you for the explanation!

[commit-bot: I haz the power, 2017-03-22 22:19:05]

CQ is committing da patch.

Bot data: {"patchset_id": 30025, "attempt_start_ts": 1490213182468750,
"parent_rev": "f7a8e76c2a53d147b214c67167661e7760d9d5f3", "commit_rev":
"437d0b9a1fbd7345b234e54e09716f501c37ccb8"}

[commit-bot: I haz the power, 2017-03-22 22:19:50]

Description was changed from

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=704243, 703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix inline focus ring paint invalidation on continuation change

An inline element's focus ring encloses its block and inline
continuations. The previous method handled invalidation of
display item client only, not rectangles covering the changed
inlines.

Now call setMayNeedPaintInvalidation() on the inline whose block
continuations are marked needsPaintOffsetAndVisualRectUpdate().

Also noticed a focus ring painting bug (crbug.com/703403) and added
a test case. Will fix later.

BUG=704243, 703403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2765763002
Cr-Commit-Position: refs/heads/master@{#458895}
Committed:
https://chromium.googlesource.com/chromium/src/+/437d0b9a1fbd7345b234e54e0971...
==========

[commit-bot: I haz the power, 2017-03-22 22:19:53]

Committed patchset #4 (id:30025) as
https://chromium.googlesource.com/chromium/src/+/437d0b9a1fbd7345b234e54e0971...