New issue
Advanced search Search tips

Issue 755750 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 779935

Blocking:
issue 636993



Sign in to add a comment

Add native support of getClientRects() and getBoundingClientRect() in Layout NG

Project Member Reported by xiaoche...@chromium.org, Aug 15 2017

Issue description

We should reimplement APIs getClientRects() and getBoundingClientRect() so that they get coordinates from NGPhysicalFragments instead of the legacy layout tree.

At the starting phase, we should add native support for text nodes: get the coordinates from NGPhysicalTextFragments instead of InlineTextBoxes.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 15 2017

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

commit 977e02eed288af1f9d732910dcd0228d57a5a373
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Aug 15 22:16:46 2017

Change tracking bug no for some tests in LayoutNG specific expectation

NOTRY=TRUE
TBR=eae@chromium.org

Bug:  755750 
Change-Id: I40c9dcf4a6103a452f8f7071d681c6b71d4622be
Reviewed-on: https://chromium-review.googlesource.com/616115
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494572}
[modify] https://crrev.com/977e02eed288af1f9d732910dcd0228d57a5a373/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

Comment 2 Deleted

Cc: xiaoche...@chromium.org
Components: Blink>Editing
Owner: ----
Status: Available (was: Assigned)
Not going to work on it shortly...

Comment 4 by yosin@chromium.org, Oct 31 2017

Blockedon: 779935
Labels: -Pri-2 Pri-3
We should have UMA for getClientRects() and getBoundingClientRect() for prioritization, see  issue 779935 .

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 27 2018

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

commit b0b7031e882448d2f7db62641926c0ff2ad55445
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Mar 27 10:06:59 2018

[LayoutNG] Implements Range.getClientRects and getBoundingClientRect

This patch implements Range.getClientRects and getBoundingClientRect
by implemeting LayoutText::AbsoluteQuadsForRange, the underlying
function to compute quads for a range of text.

VisibleUnits and TouchAdjustment are other users of this function.

 crbug.com/698038  added some logic to clear the quad vector under
specific condition for legacy, but the logic is a bit complicated.
This patch tries to match to the spec without using the same logic.
There may be a need to tweak the logic a little more, but the new
logic seems to be more interoperable with Edge/Gecko.

Bug: 636993,  755750 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: I5c6323680285a4e03d64109cd5681c788f6a6bbf
Reviewed-on: https://chromium-review.googlesource.com/979732
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546079}
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/LayoutText.h
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/LayoutTextTest.cpp
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset_rect.cc
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/ng/geometry/ng_physical_offset_rect.h
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/ng/inline/ng_physical_text_fragment.cc
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/ng/inline/ng_physical_text_fragment.h
[modify] https://crrev.com/b0b7031e882448d2f7db62641926c0ff2ad55445/third_party/WebKit/Source/core/layout/ng/inline/ng_physical_text_fragment_test.cc

Comment 6 by kojii@chromium.org, Mar 30 2018

Blocking: 636993
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 30 2018

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

commit c6733a576db579fb3630d5d80de62b43430a1fc9
Author: Koji Ishii <kojii@chromium.org>
Date: Fri Mar 30 20:57:57 2018

[LayoutNG] Fix mid-cluster case for GetClientRects

Following the initial implementation of GetClientRects in CL:979732,
this patch:

1. Fixes the mid-cluster case by adding AdjustMidCluster to
   ShapeResult::PositionForOffset().
2. Share the logic with NGCaretRect, following CL:981848.

Still GetClientRects may return incorrect number of rects. This is
to be handled in higher layer and that will be in separate patches.

Bug: 636993,  755750 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Id3962251e2b65b2aae3fedf57c1355025374a9cb
Reviewed-on: https://chromium-review.googlesource.com/987417
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547262}
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[add] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/external/wpt/css/cssom-view/cssom-getClientRects-002-expected.txt
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/core/layout/ng/inline/ng_physical_text_fragment.cc
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/core/layout/ng/inline/ng_physical_text_fragment.h
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBuffer.cpp
[modify] https://crrev.com/c6733a576db579fb3630d5d80de62b43430a1fc9/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultInlineHeaders.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 19

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

commit 08bb68ed2ad257016a3d69aa6075da1d6c11f481
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Jul 19 00:58:09 2018

Improve fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html

The layout has unclear test flow, as it uses a big test function with
different branches with different assertions for all test cases. As a
result, the expectations for the test cases are unclear.

This patch removes all branching from the test flow, so that it becomes
clear what we expect on each test case. It also reveals that LayoutNG
has different behavior and should be marked failure here.

Bug:  755750 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I7d1a22a5805cc7ad031bf3c7bc24f0b822a3a0da
Reviewed-on: https://chromium-review.googlesource.com/1142573
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576289}
[modify] https://crrev.com/08bb68ed2ad257016a3d69aa6075da1d6c11f481/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/08bb68ed2ad257016a3d69aa6075da1d6c11f481/third_party/WebKit/LayoutTests/fast/dom/Range/get-bounding-client-rect-empty-and-non-empty.html

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 20

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

commit 7a7b47134d2380dd86de75368d0777ddd8482bd4
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Jul 20 18:32:07 2018

[LayoutNG] Stop getClientRects/getBoundingClientRects from adding unnecessary collapsed rects

Currently the two functions adds rects from all text fragments intersecting
the input range, even if they only touch at boundaries. For example:

Text fragments:  ABC  DEF  GHI
Text offsets:    012  345  678
Input range: start = 3, end = 6

The current implementation returns three rects, one from each fragment,
even though the rects from "ABC" and "GHI" are collapsed.

This patch stops the inclusion of such collapsed rects; they are included
only when we can't find non-collapsed rects.

Bug:  755750 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I1ca65d1aee02e1eb33b8c72605daca4d6dbd119b
Reviewed-on: https://chromium-review.googlesource.com/1142685
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576935}
[modify] https://crrev.com/7a7b47134d2380dd86de75368d0777ddd8482bd4/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/7a7b47134d2380dd86de75368d0777ddd8482bd4/third_party/blink/renderer/core/layout/layout_text.cc

Status: Fixed (was: Available)
Can be considered as fixed.

Sign in to add a comment