New issue
Advanced search Search tips

Issue 855279 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 24
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Task

Blocked on:
issue 851075



Sign in to add a comment

[LayoutNG] Fix remaining hit test failures

Project Member Reported by xiaoche...@chromium.org, Jun 21 2018

Issue description

This is the meta bug for all remaining LayoutNG hit test failures.
 
Blockedon: 851075
Project Member

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

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

commit 5cb687223b49d440e01c1b830310f16583e8e99a
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Jun 22 19:35:34 2018

[LayoutNG] Implement hit testing on list marker

When hit testing a list marker, we should hit the corresponding list
item element. This patch implements the same behavior in LayoutNG.

Bug:  855279 
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: I4d191098a444cf276b917451ae1f9ab4a0df2d88
Reviewed-on: https://chromium-review.googlesource.com/1111270
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: cathie chen <cathiechen@tencent.com>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569735}
[modify] https://crrev.com/5cb687223b49d440e01c1b830310f16583e8e99a/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/5cb687223b49d440e01c1b830310f16583e8e99a/third_party/blink/renderer/core/layout/ng/list/layout_ng_list_marker.h
[modify] https://crrev.com/5cb687223b49d440e01c1b830310f16583e8e99a/third_party/blink/renderer/core/paint/ng/ng_paint_fragment.cc

Project Member

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

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

commit 4223de4af39412ee5b39cf5780253974edd658ac
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Jun 26 20:31:15 2018

Change baseline for ellipsis hit testing to match spec

According to spec (https://drafts.csswg.org/css-ui/#ellipsing-details):

"The UA should dispatch any pointer event on the ellipsis to the elided
element, as if text-overflow had been none."

Current layout test fast/css/text-overflow-ellipsis-vertical-hittest.html
asserts that hit testing on ellipsis doesn't hit the elided text node.
This patch changes its baseline to match the spec. It also marks it
as failure in legacy layout and pass in LayoutNG.

Bug:  855279 
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: I41e41a1072578330b7dbdf08e153d2022f59b79d
Reviewed-on: https://chromium-review.googlesource.com/1114280
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570516}
[modify] https://crrev.com/4223de4af39412ee5b39cf5780253974edd658ac/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/4223de4af39412ee5b39cf5780253974edd658ac/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4223de4af39412ee5b39cf5780253974edd658ac/third_party/WebKit/LayoutTests/fast/css/text-overflow-ellipsis-vertical-hittest-expected.txt

Project Member

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

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

commit 8e045343cea6e6c73aff6cb48c45ee1ce60659a7
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Jun 28 20:19:32 2018

[LayoutNG] Simplify offset accumulation during hit test

Currently, in both legacy and NG NodeAtPoint() variants, parameter
|accumulated_offset| stands for the offset of the parent context or
fragment in the paint layer, instead of the own offset of the
object under hit test.

This makes the parameter unintuitive, and also makes offset conversion
complicated when crossing legacy/NG boundaries.

This patch changes NG to use the own offset of the fragment under hit
test to simplify offset calculation. This also eliminates the duplicated
offset accumulation in both LayoutNGMixin and NGBoxFragmentPainter.

Bug:  855279 
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: I2256e95a7b948e107b10daf2c25206b2d32b090e
Reviewed-on: https://chromium-review.googlesource.com/1117878
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571239}
[modify] https://crrev.com/8e045343cea6e6c73aff6cb48c45ee1ce60659a7/third_party/blink/renderer/core/layout/layout_inline.cc
[modify] https://crrev.com/8e045343cea6e6c73aff6cb48c45ee1ce60659a7/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc
[modify] https://crrev.com/8e045343cea6e6c73aff6cb48c45ee1ce60659a7/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[modify] https://crrev.com/8e045343cea6e6c73aff6cb48c45ee1ce60659a7/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

Owner: xiaoche...@chromium.org
Status: Assigned (was: Available)
Project Member

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

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

commit 389b61edf66ebb76fed60629e9cb9e95a4069c09
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Jun 29 20:19:02 2018

[LayoutNG] Fix physical offset calculation in LayoutNGMixin::NodeAtPoint()

In LayoutNGMixin::NodeAtPoint(), parameter |accumulated_offset|
satisfies that, |accumulated_offset + Location()| equals the physical
offset of the current LayoutBox in the paint layer, regardless of
writing mode or whether the box was placed by NG or legacy.

This patches fixes the physical offset calculation utilizing the
above invariant. It also renames variable |adjusted_location| into
|physical_offset| to be more accurate.

Bug:  855279 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I73311ad17c56cf1f300e627966904fac3430a5d9
Reviewed-on: https://chromium-review.googlesource.com/1117881
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Aleks Totic <atotic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571608}
[modify] https://crrev.com/389b61edf66ebb76fed60629e9cb9e95a4069c09/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/389b61edf66ebb76fed60629e9cb9e95a4069c09/third_party/blink/renderer/core/layout/ng/layout_ng_mixin.cc

Project Member

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

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

commit b34fb980f7d1e306456c3cf4b986645f1b3c33f3
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Jun 29 21:13:15 2018

[LayoutNG] Add comments to and use better variable names in hit test code

NG Hit test code currently uses variable names with unclear meanings,
e.g. |accumulated_offset|, |adjusted_location|, ...., like legacy.

This patch changes them into clear terms like |physical_offset|, and
adds comments for clarification.

Bug:  855279 
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: I6c189b7e7001db8abb66241500b97d5df0c9c5b0
Reviewed-on: https://chromium-review.googlesource.com/1121052
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571630}
[modify] https://crrev.com/b34fb980f7d1e306456c3cf4b986645f1b3c33f3/third_party/blink/renderer/core/paint/ng/ng_block_flow_painter.cc
[modify] https://crrev.com/b34fb980f7d1e306456c3cf4b986645f1b3c33f3/third_party/blink/renderer/core/paint/ng/ng_block_flow_painter.h
[modify] https://crrev.com/b34fb980f7d1e306456c3cf4b986645f1b3c33f3/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[modify] https://crrev.com/b34fb980f7d1e306456c3cf4b986645f1b3c33f3/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 2

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

commit cb238dbf866deb301fad7b84ac67a2b593bc5330
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Mon Jul 02 21:03:37 2018

[LayoutNG] Cleanup and fix hit test fallback offset calculation

Currently, LayoutNG hit test code uses FallbackAccumulatedOffset()
to calculate the |accumulated_offset| parameter at two places:
1. When falling back to LayoutBox::HitTestAllPhases/NodeAtPoint
2. When falling back to LayoutInline::HitTestCulledInline

However, the parameter actually has different semantics in the two
legacy functions. This makes the function complicated and hard to
modify.

This patch breaks the function, which results in simplification at
both fallback sites. Besides, it also fixes the offset calculation
at the first site in 'vertical-rl' writing mode, making two layout
tests pass.

Bug:  855279 ,  856730 
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: Iaa59cbfbb2396f2df7c1550fca45eb4ae84e3052
Reviewed-on: https://chromium-review.googlesource.com/1119639
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571994}
[modify] https://crrev.com/cb238dbf866deb301fad7b84ac67a2b593bc5330/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/cb238dbf866deb301fad7b84ac67a2b593bc5330/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/cb238dbf866deb301fad7b84ac67a2b593bc5330/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 3

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

commit 98d4a440eebb0bffe5cd97918833090ed3493666
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Jul 03 01:40:13 2018

[LayoutNG] Fix hit test clipping by fragmented border with round corner

Current NG hit test code decides whether a hit test location is clipped
by rounded border by adding border radius to all four corners and then
compare with the hit test location. This is wrong if the box is
fragmented, where it may have only two or zero rounded corners.

This patch changes HitTestClippedOutByBorder() to take the above into
consideration. By utilizing |border_edges_|, it can generate the
corrected rounded border to be hit tested.

Test: This patch slightly modifies WPT
hit-test-inline-fragmentation-with-border-radius.html as a workaround
of some rounding issue, which is out of the scope of this patch.

Bug:  855279 , 859233
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: I1e3193f9fc2d5e52ff53d6cb2fcc2a0ae2b3aa43
Reviewed-on: https://chromium-review.googlesource.com/1123265
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572079}
[modify] https://crrev.com/98d4a440eebb0bffe5cd97918833090ed3493666/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/98d4a440eebb0bffe5cd97918833090ed3493666/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/98d4a440eebb0bffe5cd97918833090ed3493666/third_party/WebKit/LayoutTests/external/wpt/css/css-break/hit-test-inline-fragmentation-with-border-radius.html
[modify] https://crrev.com/98d4a440eebb0bffe5cd97918833090ed3493666/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 3

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

commit 555b4daae14cc2ba1be82c5fc930b1156e88345f
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Jul 03 23:45:33 2018

[LayoutNG] Add scrolled offset when hit testing scrolled content

This patch takes scrolled offset into account when hit testing the
children of a scroll container, so that scrolled contents can be hit
tested correctly.

Bug:  855279 
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: Ic84b40c06302c457cf32cacaa5e79f419dcc3a2b
Reviewed-on: https://chromium-review.googlesource.com/1125312
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572398}
[modify] https://crrev.com/555b4daae14cc2ba1be82c5fc930b1156e88345f/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/555b4daae14cc2ba1be82c5fc930b1156e88345f/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[modify] https://crrev.com/555b4daae14cc2ba1be82c5fc930b1156e88345f/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 25

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

commit 654c439b6fa991a3c834e9933092b216a41a12a5
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Jul 25 18:59:22 2018

[LayoutNG] Allow entering line boxes to hit test floats

After crrev.com/c/1132541, more floats are made children of line boxes.
To ensure their hit-testability, we must traverse into line boxes to
find them when HitTestAction is kHitTestFloat, and then hit test them
with all phases.

This patch implements the above logic.

Bug:  855279 
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: Iee6e015cb035b5b9e94fa4fab9e510839424d35d
Reviewed-on: https://chromium-review.googlesource.com/1149416
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578002}
[modify] https://crrev.com/654c439b6fa991a3c834e9933092b216a41a12a5/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/654c439b6fa991a3c834e9933092b216a41a12a5/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.cc
[modify] https://crrev.com/654c439b6fa991a3c834e9933092b216a41a12a5/third_party/blink/renderer/core/paint/ng/ng_box_fragment_painter.h

Labels: OS-Android
Status: Fixed (was: Assigned)
All layout test failures directly due to hit testing are fixed.

We should post individual bugs if any new failure is seen.

Sign in to add a comment