SelectionModifier should work with LayoutNG |
|
Issue descriptionSelectionModifier still heavily depends on legacy InlineBox. We should make it work on NG inline layout structure.
,
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
,
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
,
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
,
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
,
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
,
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 |
|
Comment 1 by bugdroid1@chromium.org
, Aug 24