New issue
Advanced search Search tips

Issue 812535 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 771398
issue 789870
issue 807930



Sign in to add a comment

Improve LocalCaretRectOfPosition

Project Member Reported by yoichio@chromium.org, Feb 15 2018

Issue description

LocalCaretRectOfPosition is half baked and doesn't work on edge cases.
The function is also inconsistent between legacy and NG.

Fix it.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 16 2018

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

commit 7db161842fbfaa5454ef8bf6474f8532750c70f0
Author: Yoichi Osato <yoichio@chromium.org>
Date: Fri Feb 16 06:46:35 2018

[LayoutNG] Add some fault test case for LocalCaretRectTest

LocalCaretRectTest returns null LayoutRect for "after" node such as
kAfterAnchor, kAfterChildren, kOffsetInAnchor and offset==length.

This patch adds such cases. I'll fix it in later patch.

Bug: 812535
Change-Id: I6349360ab6b7ff41870e05f154471436653917dc
Reviewed-on: https://chromium-review.googlesource.com/920127
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537234}
[modify] https://crrev.com/7db161842fbfaa5454ef8bf6474f8532750c70f0/third_party/WebKit/Source/core/editing/LocalCaretRectTest.cpp

Project Member

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

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

commit 1921c472495f755af20984ba849ba8d94f0abe19
Author: Yoichi Osato <yoichio@chromium.org>
Date: Thu Feb 22 02:17:31 2018

LocalCaretRectOfPosition: Remove shadow check

ComputeInlineAdjustedPosition adjusts position so that
AnchorNode is not shadow root.
However, no one call the function with shadow anchored position.

This patch removes the adjustment and just DCHECK it.

Bug: 812535
Change-Id: Idf00b013f7face181e9a8374b0e4abc85888097c
Reviewed-on: https://chromium-review.googlesource.com/926141
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538320}
[modify] https://crrev.com/1921c472495f755af20984ba849ba8d94f0abe19/third_party/WebKit/Source/core/editing/InlineBoxPosition.cpp

Project Member

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

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

commit 4b21f5cb58a5dc36b63a4d396cd1c4e0abbf8c7d
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Mar 27 04:44:32 2018

Make LocalCaretRectTest verify more linebreak related cases

LocalCaretRectTest has parameterization for LayoutNG. However, test
case |AfterLineBreak| uses contenteditable unnecessarily, which forces
legacy layout and invalidates the parameterization. This patch fixes
it.

This patch also adds two similar test cases for line breaks in
'white-space: pre' style, verifying that NG LocalCaretRect impl
handles consecutive line breaks at block end correctly.

Bug: 812535
Change-Id: Iaba4bcc616078f35f73fca23565b6628f04f7271
Reviewed-on: https://chromium-review.googlesource.com/974909
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546014}
[modify] https://crrev.com/4b21f5cb58a5dc36b63a4d396cd1c4e0abbf8c7d/third_party/WebKit/Source/core/editing/LocalCaretRectTest.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

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

commit 614797964fd241c101fa60be39803c525eca6033
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Apr 17 18:48:25 2018

Merge LocalCaretRect's line end hack into ComputeInlineBoxPosition()

As a half-baked utility function, ComputeInlineBoxPosition() used to
return null for positions after linebreak. Some callers have ad hoc
hacks to fix up such null returns.

r547639 added such a fixup in LocalCaretRectOfPosition().

This patch merges the fixup into ComputeInlineBoxPosition(), since
LocalCaretRectOfPosition() is supposed to be a wrapper of CIBP().

For changes in SelectionModifierCharacter: the class has code handling
positions after linebreaks, but misses the case of '\n' in
'white-space: pre' style, and handles the case by other fixup path.
With the merge, we no longer hit the fixup path, and have to fix the
linebreak handling in it.

Bug: 812535
Change-Id: I9308dcd2bd911b63b0d2268a2db7a0cb94e63d4c
Reviewed-on: https://chromium-review.googlesource.com/1014606
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551408}
[modify] https://crrev.com/614797964fd241c101fa60be39803c525eca6033/third_party/blink/renderer/core/editing/inline_box_position.cc
[modify] https://crrev.com/614797964fd241c101fa60be39803c525eca6033/third_party/blink/renderer/core/editing/inline_box_position.h
[modify] https://crrev.com/614797964fd241c101fa60be39803c525eca6033/third_party/blink/renderer/core/editing/local_caret_rect.cc
[modify] https://crrev.com/614797964fd241c101fa60be39803c525eca6033/third_party/blink/renderer/core/editing/selection_modifier_character.cc

Owner: ----
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 11

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

commit 434458ed0436b62e2f395864f050bc752fd6d5a1
Author: Yoichi Osato <yoichio@chromium.org>
Date: Tue Sep 11 04:26:02 2018

Fix line end caret w/o any following LayoutObject.

This patch fix ComputeInlineBoxPosition for the position that
points after line break and no LayoutObject after that by
fallback to previous position.
The fallback examples:
- "<br>|" to "|<br>"
- "<pre>foo\n|</pre>" to "<pre>foo|\n</pre>"

This change aligns legacy LocalCaretRect to NG.

Bug: 789870, 812535
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Icb6cd9b8bc58b56a04d01eb22bdd58bd16ad5a01
Reviewed-on: https://chromium-review.googlesource.com/1215506
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590199}
[modify] https://crrev.com/434458ed0436b62e2f395864f050bc752fd6d5a1/third_party/blink/renderer/core/editing/inline_box_position.cc
[modify] https://crrev.com/434458ed0436b62e2f395864f050bc752fd6d5a1/third_party/blink/renderer/core/editing/local_caret_rect_test.cc

Sign in to add a comment