New issue
Advanced search Search tips

Issue 889449 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 13
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[LayoutNG] Support scroll anchoring

Project Member Reported by mstensho@chromium.org, Sep 26

Issue description

Make sure scroll anchoring works as well in LayoutNG as in legacy layout.

Tests: third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-anchoring/
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 26

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

commit c5bc50c28979f75ba5fdeb66c3a12371c9ea7f59
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Wed Sep 26 17:51:18 2018

[LayoutNG] Parameterize scroll anchoring unit tests.

We want to test this with LayoutNG.
Skip failing tests on NG (there are two).

Bug:  889449 
Change-Id: I3b80eb07d9dc359d24ab63417c3ca2ac228c1496
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/1245793
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594378}
[modify] https://crrev.com/c5bc50c28979f75ba5fdeb66c3a12371c9ea7f59/third_party/blink/renderer/core/layout/scroll_anchor_test.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 27

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

commit 27644ac61147a9a8dc22a05b7d83b9e6474bb6d0
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Thu Sep 27 06:51:15 2018

[LayoutNG] Handle text objects correctly for scroll anchoring.

Use LayoutText::LinesBoundingBox() (which knows how to deal with both NG
and legacy layout), rather than reading out from the legacy line box
tree manually.

Added a crash unit test that would have crashed with this CL, had it not been
for https://chromium-review.googlesource.com/1193868 - we used to stumble
into dead layout objects.
This CL also fixes one existing unit test.

Bug:  889449 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Icb3f75038e1504968badaedd992565c8c1cc8419
Reviewed-on: https://chromium-review.googlesource.com/1158688
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594616}
[modify] https://crrev.com/27644ac61147a9a8dc22a05b7d83b9e6474bb6d0/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/27644ac61147a9a8dc22a05b7d83b9e6474bb6d0/third_party/blink/renderer/core/layout/scroll_anchor.cc
[modify] https://crrev.com/27644ac61147a9a8dc22a05b7d83b9e6474bb6d0/third_party/blink/renderer/core/layout/scroll_anchor_test.cc

Owner: mstensho@chromium.org
Status: Assigned (was: Available)
Still one failing unit test.
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 8

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

commit aebe42cafcdb6c49a4c46278791e1aae3300b8c8
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Mon Oct 08 12:53:36 2018

[LayoutNG] Correct LayoutText::LinesBoundingBox() for vertical-rl.

NG needs to produce a rectangle with the block-axis offset relatively to
block-start of the container, just like legacy does. Using purely
physical coordinates is wrong.

This fixes issues with scroll anchoring, and possibly other things too.

One test had to be rebaselined, due to a rounding change.
LayoutText::DebugRect() calls LinesBoundingBox(), which now flips the
block-axis coordinate for vertical-rl. This affects rounding of the
size of the rectangle.

Bug:  889449 
Change-Id: I7053ed7fe05ce443f53a128660d60f27fea7f8b3
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/c/1264596
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597541}
[add] https://crrev.com/aebe42cafcdb6c49a4c46278791e1aae3300b8c8/third_party/WebKit/LayoutTests/external/wpt/css/css-scroll-anchoring/text-anchor-in-vertical-rl.html
[modify] https://crrev.com/aebe42cafcdb6c49a4c46278791e1aae3300b8c8/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/html/details_summary/details-marker-style-expected.txt
[modify] https://crrev.com/aebe42cafcdb6c49a4c46278791e1aae3300b8c8/third_party/blink/renderer/core/layout/layout_text.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 13

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

commit 3b21ced5db452b1cfcb60f39758f208b31bead9c
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue Nov 13 13:38:06 2018

[LayoutNG] Enable final unit test for scroll anchoring.

It was disabled, because it used to fail. Now it passes. Don't remember
the details.

Bug:  889449 
Change-Id: I0cd4b0f6fa5bb37494e814818740b3e35afc6fbe
Reviewed-on: https://chromium-review.googlesource.com/c/1333374
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607576}
[modify] https://crrev.com/3b21ced5db452b1cfcb60f39758f208b31bead9c/third_party/blink/renderer/core/layout/scroll_anchor_test.cc

Status: Fixed (was: Assigned)

Sign in to add a comment