New issue
Advanced search Search tips

Issue 901492 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Word boundary issues with text control elements

Project Member Reported by xiaoche...@chromium.org, Nov 2

Issue description

Chrome Version: 72.0.3599.0 Canary
OS: Linux

What steps will reproduce the problem?
(1) Open http://jsfiddle.net/bxfk0285/
(2) Select "foo", and then press Ctrl+Shift+Right for multiple times
(3) Select "bar", and then press Ctrl+Shift+Left for multiple times
(4) Unselect, and then double click on "bar"

What is the expected result?

(2) Selection is extended to "bar"
(3) Selection is extended to "foo"
(4) "bar" should be selected

What happens instead?

(2) Selection can't be extended
(3) Selection can't be extended
(4) "bla" inside text control is selected

Note: This is a bug related to TextOffsetMapping. (3) doesn't reproduce on M70 Stable because PreviousWordPosition() starts to use TextOffsetMapping in M72.
 
Cc: yosin@chromium.org
Labels: -Pri-2 Pri-1
Something seems wrong in how TextOffsetMapping::InlineContents divides the layout tree.

Currently, TextOffsetMapping::InlineContents enters inline blocks (or floats, OOF-positioned) unconditionally. However, we don't want to enter text control elements when searching for granularity boundaries.

Consider running example "foo<input placeholder=bla>bar". We use placeholder so that "bla" is uneditable, and editing boundary adjustment doesn't help.

Idea 1: When computing InlineContents range, we always break a block flow before/after text control elements, and don't enter text controls.

In this way, we get InlineContents "foo" and "bar". However this may break sentence boundary finding, as TextSegments treats InlineContents boundaries as block boundaries

Idea 2: InlineContents should also state its boundary type:
- boundary of inline formatting context
- entering/leaving a child block formatting context, where the child can be inline block, float or oof-positioned.
- entering/leaving a parent inline formatting context

In this way, sentence boundary algorithms can decide whether to stop or continue when reaching InlineContents boundaries.

Comment 2 Deleted

Wait... Actually, sentence boundary algorithms should also treat "normal" inline block boundaries as block boundaries, so Idea 1 doesn't break sentence boundary algorithm.

We only need to make sure
1. InlineContents range iteration doesn't cross text control boundaries.
2. TextOffsetMapping shouldn't expose contents of text control elements; instead it should treat text control element as an object replacement character.

We may probably want to do the same for replaced elements.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 9

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

commit 4d281c8de3c433b6660a63af2b33977800d94aca
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Nov 09 05:26:21 2018

Make InlineContents traverse flat tree for previous and next

Currently, InlineContents creation and previous/next movement are done
by different implementations:

- When creating from a position, it traverses the flat tree until
  finding a node in a usable block flow;
- When moving previous/next, it traverses the layout tree until finding
  a useable block flow.

Since these operations are very similar, this patch change the latter
to also traverse on flat tree and share the traversal algorithm to
reduce duplicated logic. This also makes it easier to change
TextOffsetMapping not to cross text control boundaries (*).

(*) crrev.com/c/1327962

Bug:  901492 
Change-Id: If7c3dd262903c1a0f81166f729614a8d810f5d27
Reviewed-on: https://chromium-review.googlesource.com/c/1327531
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606748}
[modify] https://crrev.com/4d281c8de3c433b6660a63af2b33977800d94aca/third_party/blink/renderer/core/editing/text_offset_mapping.cc
[modify] https://crrev.com/4d281c8de3c433b6660a63af2b33977800d94aca/third_party/blink/renderer/core/editing/text_offset_mapping.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 13

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

commit e349a45e258782e8e00713b0690f38d6001fc650
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Nov 13 22:52:59 2018

Make TextOffsetMapping not cross text control boundaries

This patch solves the issue of crossing text control boundaries when
using TextOffsetMapping. The changes include:

1. Change TextOffsetMapping::FindForward/BackwardInlineContents() not to
   cross text control boundaries:
   - If the initial position is in text control, keep traversal inside
     the text control
   - If the initial position is outside text control, all descendants of
     text control elements are skipped

2. Change flat tree TextIterator to respect the EntersTextControls flag,
   so that it doesn't dump the internal text of text controls elements
   to TextOffsetMapping, so that TextOffsetMapping won't give a result
   boundary inside text control.

Many test cases are also added to TextIterator, TextOffsetMapping and
word/sentence boundary algorithms.

Bug:  901492 
Change-Id: I61aa3d3679eb77b975b50fd52dc8f79e049d8e47
Reviewed-on: https://chromium-review.googlesource.com/c/1327962
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607787}
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/dom/flat_tree_traversal.h
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/iterators/text_iterator.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/iterators/text_iterator_test.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/text_offset_mapping.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/text_offset_mapping.h
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/text_offset_mapping_test.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/visible_units_sentence_test.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/editing/visible_units_word_test.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/html/forms/text_control_element.cc
[modify] https://crrev.com/e349a45e258782e8e00713b0690f38d6001fc650/third_party/blink/renderer/core/html/forms/text_control_element.h

Status: Fixed (was: Assigned)

Sign in to add a comment