New issue
Advanced search Search tips

Issue 850448 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Task

Blocking:
issue 707656



Sign in to add a comment

Document Marker should work with LayoutNG

Project Member Reported by yoichio@chromium.org, Jun 7 2018

Issue description

Make LayoutTests/paint/markers/ pass on NG.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d474821aac3e0c124e2da7d0b26fa308193172d

commit 4d474821aac3e0c124e2da7d0b26fa308193172d
Author: Yoichi Osato <yoichio@chromium.org>
Date: Wed Jun 13 09:21:04 2018

[LayoutNG] Paint composition document marker

This patch imports the paint logic from InlineTextBoxPainter, converts DOM offset
to inline offset and paint markers with NGPhysicalTextFragment.

Here is copied functions from InlinTextBoxPainter
to NGTextFragmentPainter:
 PaintSingleMarkerBackgroundRun
 PaintStyleableMarkerUnderline
 PaintDocumentMarkers

This patch fixes:
paint/markers/active-suggestion-marker-basic.html
paint/markers/composition-marker-basic.html

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_layout_ng;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I6126bcc3880fe8ba9bd0ebd6262bae4cd7b762be
Reviewed-on: https://chromium-review.googlesource.com/1090601
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566769}
[modify] https://crrev.com/4d474821aac3e0c124e2da7d0b26fa308193172d/third_party/blink/renderer/core/editing/markers/document_marker_controller.h
[modify] https://crrev.com/4d474821aac3e0c124e2da7d0b26fa308193172d/third_party/blink/renderer/core/paint/BUILD.gn
[add] https://crrev.com/4d474821aac3e0c124e2da7d0b26fa308193172d/third_party/blink/renderer/core/paint/document_marker_painter.h
[modify] https://crrev.com/4d474821aac3e0c124e2da7d0b26fa308193172d/third_party/blink/renderer/core/paint/inline_text_box_painter.cc
[modify] https://crrev.com/4d474821aac3e0c124e2da7d0b26fa308193172d/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50dc960479f0d17e8a8bbc37453c8356c1e8e551

commit 50dc960479f0d17e8a8bbc37453c8356c1e8e551
Author: Mostyn Bramley-Moore <mostynb@vewd.com>
Date: Wed Jun 13 13:48:34 2018

[jumbo] fixup after LayoutNG work

https://chromium-review.googlesource.com/1090601 copied an enum
class declaration into some LayoutNG files, which breaks jumbo
builds which see both declarations at the same time.  Let's just
include the original.

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id219bdd6a059ac1501be3d912da71468f7d3797c
Reviewed-on: https://chromium-review.googlesource.com/1098674
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com>
Cr-Commit-Position: refs/heads/master@{#566820}
[modify] https://crrev.com/50dc960479f0d17e8a8bbc37453c8356c1e8e551/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf2eee8f9799b282158a98ef55180d00fb8e3db2

commit cf2eee8f9799b282158a98ef55180d00fb8e3db2
Author: Yoichi Osato <yoichio@chromium.org>
Date: Thu Jun 14 07:36:00 2018

[LayoutNG] Rebase inline-spelling-markers-hidpi.html image expecation.

This patch rebases image expectations for
- paint/markers/inline-spelling-markers-hidpi-composited.html
- paint/markers/inline-spelling-markers-hidpi.html
since they are scaled and only have 1px diff.

Bug:  850448 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Ib5e68393d577a7ee493b31406d65dab60efb36ce
Reviewed-on: https://chromium-review.googlesource.com/1100595
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567181}
[modify] https://crrev.com/cf2eee8f9799b282158a98ef55180d00fb8e3db2/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/inline-spelling-markers-hidpi-composited-expected.png
[modify] https://crrev.com/cf2eee8f9799b282158a98ef55180d00fb8e3db2/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/inline-spelling-markers-hidpi-expected.png

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b130efd45d217f8ddd2e534cbbaf59041478b94b

commit b130efd45d217f8ddd2e534cbbaf59041478b94b
Author: Yoichi Osato <yoichio@chromium.org>
Date: Fri Jun 15 09:33:22 2018

[LayoutNG] Paint spelling/grammar markers

This patch implements painting spelling/grammar markers by
sharing logic from InlineTextBoxPainter.

This patch fixes:
- paint/markers/marker-early-break-bug.html

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I4ebb3c788c614fde25c5fc28d461b089edb57fa9
Reviewed-on: https://chromium-review.googlesource.com/1100669
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567591}
[modify] https://crrev.com/b130efd45d217f8ddd2e534cbbaf59041478b94b/third_party/blink/renderer/core/paint/document_marker_painter.h
[modify] https://crrev.com/b130efd45d217f8ddd2e534cbbaf59041478b94b/third_party/blink/renderer/core/paint/inline_text_box_painter.cc
[modify] https://crrev.com/b130efd45d217f8ddd2e534cbbaf59041478b94b/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5fc99d98ff3ae18b4f8b01eede902f8a3188256

commit d5fc99d98ff3ae18b4f8b01eede902f8a3188256
Author: Yoichi Osato <yoichio@chromium.org>
Date: Mon Jun 18 11:10:04 2018

[LayoutNG] Paint text match markers

This patch implements painting text match markers.
Logic is copied from inline_text_box_painter.cc, which is very simple:
1 paint background rect with a given color
2 paint foreground text with a given color

This patch fixes:
paint/invalidation/text-match-pre-wrapped-text.html
paint/invalidation/text-match-transparent-text.html
paint/invalidation/text-match.html
paint/markers/first-letter.html

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ic09ef1748620d5d353f89fc5c8ebced768d5a5ed
Reviewed-on: https://chromium-review.googlesource.com/1103990
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567969}
[modify] https://crrev.com/d5fc99d98ff3ae18b4f8b01eede902f8a3188256/third_party/blink/renderer/core/paint/document_marker_painter.h
[modify] https://crrev.com/d5fc99d98ff3ae18b4f8b01eede902f8a3188256/third_party/blink/renderer/core/paint/inline_text_box_painter.cc
[modify] https://crrev.com/d5fc99d98ff3ae18b4f8b01eede902f8a3188256/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1805b4dc1d63e92bab0c8d884790096573fcba36

commit 1805b4dc1d63e92bab0c8d884790096573fcba36
Author: Yoichi Osato <yoichio@chromium.org>
Date: Thu Jun 21 04:27:18 2018

[LayoutNG] Not paint document marker on ellipsis.

This patch stops painting document markers on ellipsis character.

Old implementation is bit strange : painting background of ellipsis as
culled characters are not hided only for find-in-page marker. See:
https://docs.google.com/document/d/1Osd-W9IMxHrrgJ2Kvo7HfpVTms7EDUbOHhsmL1c3_Yo/

This patch implements simply: do not paint any marker on ellipsis
because characters are hide.
That can cause a bit weird situation: if find-in-page hits such hide texts,
it shows numbers of found texts but blink doesn't paint anything.
However old implementation already has similar situation: if found text are
clipped by text-overflow: clip, also blink doesn't show them.
Thus this is acceptable change.

This patch fixes following tests:
paint/markers/ellipsis-ltr-text-in-ltr-flow-with-markers.html
paint/markers/ellipsis-ltr-text-in-rtl-flow-with-markers.html
paint/markers/ellipsis-mixed-text-in-ltr-flow-with-markers.html
paint/markers/ellipsis-mixed-text-in-rtl-flow-with-markers.html
paint/markers/ellipsis-rtl-text-in-ltr-flow-with-markers.html
paint/markers/ellipsis-rtl-text-in-rtl-flow-with-markers.html

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ibdd7d485e2b5ac4b5db665d54b64131e6c1bcdb9
Reviewed-on: https://chromium-review.googlesource.com/1105681
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569152}
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-ltr-text-in-ltr-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-ltr-text-in-ltr-flow-with-markers-expected.txt
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-ltr-text-in-rtl-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-ltr-text-in-rtl-flow-with-markers-expected.txt
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-mixed-text-in-ltr-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-mixed-text-in-ltr-flow-with-markers-expected.txt
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-mixed-text-in-rtl-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-mixed-text-in-rtl-flow-with-markers-expected.txt
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-rtl-text-in-ltr-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-rtl-text-in-ltr-flow-with-markers-expected.txt
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-rtl-text-in-rtl-flow-with-markers-expected.png
[add] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/ellipsis-rtl-text-in-rtl-flow-with-markers-expected.txt
[modify] https://crrev.com/1805b4dc1d63e92bab0c8d884790096573fcba36/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1830a789acfb86a709d1cf2fec9982c0571cb303

commit 1830a789acfb86a709d1cf2fec9982c0571cb303
Author: Yoichi Osato <yoichio@chromium.org>
Date: Wed Jun 27 04:10:36 2018

[LayoutNG]: Move marker painting functions to new document_marker_painter.cc

This patch moves marker painting functions to new document_marker_painter.cc.
This patch is pure-refactoring.

Here are target functions:
DocumentMarkerController::
  ComputeMarkersToPaint
DocumentMarkerPainter::
  PaintStyleableMarkerUnderline
  PaintDocumentMarker
  ComputeTextPaintStyleFrom

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:linux_layout_tests_layout_ng

Change-Id: Ibd0c002fa820e18ff03e3ec480e0151bb197b219
Reviewed-on: https://chromium-review.googlesource.com/1114532
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570659}
[modify] https://crrev.com/1830a789acfb86a709d1cf2fec9982c0571cb303/third_party/blink/renderer/core/editing/markers/document_marker_controller.cc
[modify] https://crrev.com/1830a789acfb86a709d1cf2fec9982c0571cb303/third_party/blink/renderer/core/paint/BUILD.gn
[add] https://crrev.com/1830a789acfb86a709d1cf2fec9982c0571cb303/third_party/blink/renderer/core/paint/document_marker_painter.cc
[modify] https://crrev.com/1830a789acfb86a709d1cf2fec9982c0571cb303/third_party/blink/renderer/core/paint/document_marker_painter.h
[modify] https://crrev.com/1830a789acfb86a709d1cf2fec9982c0571cb303/third_party/blink/renderer/core/paint/inline_text_box_painter.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdf6a8b74b61a022c931d99100939b699314b85d

commit fdf6a8b74b61a022c931d99100939b699314b85d
Author: Yoichi Osato <yoichio@chromium.org>
Date: Tue Jul 03 07:44:18 2018

[LayoutNG]: Paint document markers on vertical-lr.

This patch fixes document marker painting on writing-mode:vertical-lr.

We paint document markers at 2 phases: background and foreground.
At the background phase, we paint find-in-page highlight or so on. This is done
before graphics context rotation following writing-mode.
At the foreground phase, we paint spellcheck markers. This is done
after the rotation.

Since the background phase painting needs only physical rect of target range on
the text fragment. It has worked.
However foreground painting needs a rotated rect because it is after the rotation.

This patch fixes the foreground painting by adding MarkerRectForForeground
function that returns layout rect.

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If6e401028536cc5412be420f6600b978677af98a
Reviewed-on: https://chromium-review.googlesource.com/1109663
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572139}
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/vertical-lr-expected.png
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/vertical-lr-expected.txt
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/paint/markers/vertical-lr-expected.png
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/paint/markers/vertical-lr-expected.txt
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/paint/markers/vertical-lr.html
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/platform/linux/paint/markers/vertical-lr-expected.png
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/platform/mac/paint/markers/vertical-lr-expected.png
[add] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/WebKit/LayoutTests/platform/mac/paint/markers/vertical-lr-expected.txt
[modify] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.cc
[modify] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h
[modify] https://crrev.com/fdf6a8b74b61a022c931d99100939b699314b85d/third_party/blink/renderer/core/paint/ng/ng_text_fragment_painter.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83f8880e309275a7ca68e42c454570093e21afa9

commit 83f8880e309275a7ca68e42c454570093e21afa9
Author: Yoichi Osato <yoichio@chromium.org>
Date: Mon Jul 09 03:19:51 2018

[LayoutNG] Add vertical-rl test for painting document markers.

This patch adds a layout test that confirms document markers on
 writing-mode: vertical-rl. It is already working on LayoutNG.
vertical-lr one was already shipped with:
https://chromium-review.googlesource.com/c/chromium/src/+/1109663

Bug:  850448 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I422273cf1fd95d215cb4e35fdab4ca41d1421843
Reviewed-on: https://chromium-review.googlesource.com/1125593
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573206}
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/vertical-rl-expected.png
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/paint/markers/vertical-rl-expected.txt
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/paint/markers/vertical-rl-expected.png
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/paint/markers/vertical-rl-expected.txt
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/paint/markers/vertical-rl.html
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/platform/linux/paint/markers/vertical-rl-expected.png
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/platform/mac/paint/markers/vertical-rl-expected.png
[add] https://crrev.com/83f8880e309275a7ca68e42c454570093e21afa9/third_party/WebKit/LayoutTests/platform/mac/paint/markers/vertical-rl-expected.txt

Status: Fixed (was: Started)
So far I've fixed all compatibility issues between old and LayoutNG.
If we find further task, let's open individual task.

Sign in to add a comment