New issue
Advanced search Search tips

Issue 877263 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 707656



Sign in to add a comment

SelectionModifier should work with LayoutNG

Project Member Reported by xiaoche...@chromium.org, Aug 23

Issue description

SelectionModifier still heavily depends on legacy InlineBox.

We should make it work on NG inline layout structure.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 24

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

commit 11cd2e954761f6aa25023eb98e95b3e1dc163886
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Aug 24 05:30:57 2018

Refactor SelectionModifier's internal function DirectionOf()

This patch refactors the function by extracing DirectionAt() out of it,
so that the logic of getting direction from VisiblePosition is
abstracted. It also reduces one client of InlineBox, making it easier to
transition to LayoutNG.

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ifba556e3fb03b7bfa6e4ed27df5e39d07a45eb7c
Reviewed-on: https://chromium-review.googlesource.com/1187630
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585701}
[modify] https://crrev.com/11cd2e954761f6aa25023eb98e95b3e1dc163886/third_party/blink/renderer/core/editing/selection_modifier.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 24

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

commit 7f33da45163ef62e91db336f0891d049366e309a
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Aug 24 20:55:18 2018

[EditingNG] Add NG version to SelectionModifier's DirectionAt()

This patch adds an NG version to DirectionAt(), so that SelectionModifier
can get the correct selection direction in bidi text laid out by LayoutNG.

We currently don't have layout test verifying the behavior. This patch
adds one, and makes it pass with EditingNG flag enabled.

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: If28b9735818d835bdb5ccbae50aa6fc2d75bff58
Reviewed-on: https://chromium-review.googlesource.com/1187719
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585986}
[add] https://crrev.com/7f33da45163ef62e91db336f0891d049366e309a/third_party/WebKit/LayoutTests/editing/selection/modify_move/range_selection_move_left_right_bidi.html
[modify] https://crrev.com/7f33da45163ef62e91db336f0891d049366e309a/third_party/blink/renderer/core/editing/selection_modifier.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 28

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

commit cf57ac48bc995351dd69017da65f5f347fb2a142
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Aug 28 03:39:50 2018

Improve readability in selection_modify_character.cc

This patch renames certain functions and variable names in the file to
make it more readable:
- CaretEndOffsetOf   => CaretBackwardOffsetOf
- CaretStartOffsetOf => CaretForwardOffsetOf
- CaretMinOffsetOf   => CaretForwardOffsetInLineDirection
- LogicalStartBoxOf  => LogicalForwardMostInLine
- primary_direction  => line_direction
- IsAfterAtomicInlineOrLineBreak => IsBeforeAtomicInlineOrLineBreak

This patch is also for preparation of merging bidi-related code in this
file into inline_box_traversal.cc for NG generalization. For this
purposes, it reorganizes the functions by group bidi-related functions
together.

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I2e758c5f78ec35c9e137a524c77ff08d8a2ecdf7
Reviewed-on: https://chromium-review.googlesource.com/1191191
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586571}
[modify] https://crrev.com/cf57ac48bc995351dd69017da65f5f347fb2a142/third_party/blink/renderer/core/editing/selection_modifier_character.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 29

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

commit 5f17ca980ec54f77df0a23a2b697cb621a4cdeef
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Aug 29 17:57:12 2018

Simplify the nested loops in SelectionModifierCharacter

SelectionModifierCharacter's main function, TraverseInternal, contains
two nested loops without clear loop variable or terminate condition,
making it very hard to understand.

This patch refactors it so that:

1. Introduce an InlineBoxPosition loop variable that is modified only
at the end of each iteration of the outer loop, so that it becomes
clear what we are working on in each iteration.

2. Changes the inner loop to have exactly one iteration, which has in
fact flattened the inner loop. This patch intentionally keeps the loop
statement to minimize changes and emphasize that this patch is pure
refactoring.

Regarding correctness of 2: Without patch, each iteration of the inner
loop has three possibilities, depending on the initial value of the
(box, offset) pair:

I. Returns directly, which remains the same with patch.

II. Changes value of (box, offset) and continues. With patch, it sets
the value of (next_box, next_offset), and then sets |can_create_position|
to false to instruct the outer loop to change the loop variable value
without any extra work.

III. Changes value of (box, offset) and breaks, which ends up
- Creating a Position |p| and test if it can be returned
- If not, use |p| to create next iteration's (box, offset) value
With patch, it sets |can_create_position| to true so that the same
|p| is created and tested, and if |p| is not the return value, also
uses |p| to set up the value of next iteration's |box_position|.

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Iad30a37b1ef209170ed6dd770862c13e40320f86
Reviewed-on: https://chromium-review.googlesource.com/1194964
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587187}
[modify] https://crrev.com/5f17ca980ec54f77df0a23a2b697cb621a4cdeef/third_party/blink/renderer/core/editing/inline_box_position.h
[modify] https://crrev.com/5f17ca980ec54f77df0a23a2b697cb621a4cdeef/third_party/blink/renderer/core/editing/selection_modifier_character.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 12

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

commit 31db9c945d55e3232920413a48a5f426dcb6e6fe
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Sep 12 03:54:53 2018

Rewrite & improve editing/caret/caret-direction-auto.html

The layout test is badly written that:
- It involves many operations in the test, including selection moving,
  text insertion and caret position testing, making it hard to see the
  purpose of the test.
- It uses |textInputController.firstRectForCharacterRange()| in an
  unintuitive way. While the test claims to be testing positions of
  carets, it passes non-empty ranges to the API, making the meaning of
  the return value unclear.

This patch rewrites the test that:
- It tests caret positions only. Text insertion and selection changes
  are removed since they are covered by other tests.
- It uses the more intuitive |internals.absoluteCaretBounds()| API.

Besides, this patch also converts the test with W3C testharness, to
reduces the usage of deprecated js-test.js.

This patch is preparation for removing 'unicode-bidi: plaintext' hacks
from inline_box_traversal.cc.

Bug: 877263
Change-Id: Id98e84ac875b7a2777e66cb33a491b3b8eb94a1b
Reviewed-on: https://chromium-review.googlesource.com/1217916
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@{#590593}
[delete] https://crrev.com/9185a5f2e79084fef5ce3a737df69efa87ab975a/third_party/WebKit/LayoutTests/editing/caret/caret-direction-auto-expected.txt
[modify] https://crrev.com/31db9c945d55e3232920413a48a5f426dcb6e6fe/third_party/WebKit/LayoutTests/editing/caret/caret-direction-auto.html

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 12

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

commit 99c6b1aa507e04c9285dd5dc7eaa8901c46a57a3
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Sep 12 04:31:58 2018

Remove 'unicode-bidi: plaintext' hack from bidi adjustment

With legacy layout, there is no reliable way to get the paragraph
direction when 'unicode-bidi: plaintext' style is set. The current
implementation uses some hack to cancel adjustment in this case.

This patch uses a better workaround:
- When the style is not set, use block direction
- Otherwise, use the direction with the lowest bidi level in the line

With the workaround, the patch makes the hacky part restricted in
ParagraphDirection(), and removes all relevant hacks that are exposed,
making the implementation cleaner and more consistent.

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: Ib8e64be456432c4984eedf4a8ee5f0065961bd00
Reviewed-on: https://chromium-review.googlesource.com/1217918
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590598}
[modify] https://crrev.com/99c6b1aa507e04c9285dd5dc7eaa8901c46a57a3/third_party/WebKit/LayoutTests/editing/caret/caret-direction-auto.html
[modify] https://crrev.com/99c6b1aa507e04c9285dd5dc7eaa8901c46a57a3/third_party/blink/renderer/core/editing/inline_box_position.cc
[modify] https://crrev.com/99c6b1aa507e04c9285dd5dc7eaa8901c46a57a3/third_party/blink/renderer/core/editing/inline_box_traversal.cc
[modify] https://crrev.com/99c6b1aa507e04c9285dd5dc7eaa8901c46a57a3/third_party/blink/renderer/core/editing/inline_box_traversal.h

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 24

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

commit dbf7a76b18c92d6aced27f993eea709441c916bd
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Mon Sep 24 22:40:07 2018

Fix hit test locations in RTL HitTestbingBidiTest

In RTL HitTestbingBidiTest test cases, the hit test locations are
wrong. For example:

<div dir=rtl>ABC</div> is rendered as:

+-------------------------+
|                      ABC|
+-------------------------+
^                      ^
|                      |
div->OffsetLeft()      text left edge

The current test cases misuse |div->offsetLeft()| as text left edge,
which is fixed by this patch.

Test expectations are updated by script jsfiddle.net/tLwhcv9u/39/

Bug: 877263
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I4fd1f855a5e97fa9c2ce69c68a1d30036bef652b
Reviewed-on: https://chromium-review.googlesource.com/1240854
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593720}
[modify] https://crrev.com/dbf7a76b18c92d6aced27f993eea709441c916bd/third_party/blink/renderer/core/editing/hit_testing_bidi_test.cc

Sign in to add a comment