Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883163002:
Un-insanify first-letter handling in TextIterator (to some degree) (Closed)
DescriptionUn-insanify first-letter handling in TextIterator (to some degree)
Background: (Once upon a time) TextIterator used to work on :first-letter
in some cases, with several flawed designs magically cancelling each other's
flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it
also built another level of hack over all the existing flawed designed in
TextIterator, so that the class works in more cases.
This patch removes the hack added by [1] and makes TextIterator work with
:first-letter in an understandable way:
1. Fix the logic when TextIterator decides:
- Whether text in :first-letter should be emitted; Existing implementation emits
:first-letter only when |offset_ == 0|, which is wrong if the iteration starts in
:first-letter..
- Whether the iteration should end in :first-letter; Existing implementation has
no handling of this at all.
2. When advancing from a text node's :first-letter to remaining text,
|offset_| should be set to the starting offset of the remaining text, instead
of 0.
3. Fix the logic when dealing with DOM and layout offsets in a text node
with :first-letter. When a text node has :first-letter, offsets in DOM and
layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox()
should be aware of and handle the difference at all time; Existing implementation
has no handling of the difference at all.
New unit tests have also been added for the correct behavior.
This patch is also a preparation for abstracting the text box handling logic
out of TextIterator.
[1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f
BUG=721957
TEST=TextIteratorTest.*
Review-Url: https://codereview.chromium.org/2883163002
Cr-Commit-Position: refs/heads/master@{#472368}
Committed: https://chromium.googlesource.com/chromium/src/+/d91d54ced7ed0c79f8140e232f4b6b80efb5921d
Patch Set 1 #Patch Set 2 : Rebaseline #
Total comments: 2
Patch Set 3 : Tue May 16 16:31:00 PDT 2017 #
Total comments: 1
Messages
Total messages: 43 (32 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) BUG= ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch aims at removing all these hacks and make TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ==========
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch aims at removing all these hacks and make TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes all these hacks and make TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ==========
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes all these hacks and make TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ==========
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG= ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way: 1. Fix the logic when TextIterator decides: - Whether text in :first-letter should be emitted - Whether the iteration should end in :first-letter 2. When advancing from a text node's :first-letter to remaining text, |offset_| should be set to the starting offset of the remaining text, instead of 0. 3. Fix the logic when dealing with DOM and layout offsets in a text node with :first-letter. When a text node has :first-letter, offsets in DOM and layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox() should be aware of and handle the difference at all time. New unit tests have also been added for the correct behavior. This patch is also a preparation for abstracting the text box handling logic out of TextIterator. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG=721957 TEST=TextIteratorTest.* ==========
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way: 1. Fix the logic when TextIterator decides: - Whether text in :first-letter should be emitted - Whether the iteration should end in :first-letter 2. When advancing from a text node's :first-letter to remaining text, |offset_| should be set to the starting offset of the remaining text, instead of 0. 3. Fix the logic when dealing with DOM and layout offsets in a text node with :first-letter. When a text node has :first-letter, offsets in DOM and layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox() should be aware of and handle the difference at all time. New unit tests have also been added for the correct behavior. This patch is also a preparation for abstracting the text box handling logic out of TextIterator. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG=721957 TEST=TextIteratorTest.* ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way: 1. Fix the logic when TextIterator decides: - Whether text in :first-letter should be emitted; Existing implementation emits :first-letter only when |offset_ == 0|, which is wrong if the iteration starts in :first-letter.. - Whether the iteration should end in :first-letter; Existing implementation has no handling of this at all. 2. When advancing from a text node's :first-letter to remaining text, |offset_| should be set to the starting offset of the remaining text, instead of 0. 3. Fix the logic when dealing with DOM and layout offsets in a text node with :first-letter. When a text node has :first-letter, offsets in DOM and layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox() should be aware of and handle the difference at all time; Existing implementation has no handling of the difference at all. New unit tests have also been added for the correct behavior. This patch is also a preparation for abstracting the text box handling logic out of TextIterator. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG=721957 TEST=TextIteratorTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + dmazzoni@chromium.org
dmazzoni@: Could you help me verify a behavioral change related AX? Thanks! https://codereview.chromium.org/2883163002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/first-letter-text-transform-causes-crash-expected.txt (left): https://codereview.chromium.org/2883163002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/first-letter-text-transform-causes-crash-expected.txt:12: AXRole: AXInlineTextBox "Dt" dmazzoni@: Is it that AX is depending on incorrect behavior of TextIterator? When with first-letter, AXInlineTextBox seems to hold only the text box of first-letter. Before this patch, TextIterator cannot correctly stop inside first-letter, so it emits "Dt" even if the given range is just "D" (the first-letter). With the patch, TextIterator only emits "D" when the given range is the first-letter. However, it makes the AX tree dump look wrong...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
yosin@: PTAL at this one since you approved the previous patch. Thanks!
lgtm https://codereview.chromium.org/2883163002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/accessibility/first-letter-text-transform-causes-crash-expected.txt (left): https://codereview.chromium.org/2883163002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/accessibility/first-letter-text-transform-causes-crash-expected.txt:12: AXRole: AXInlineTextBox "Dt" On 2017/05/16 03:50:06, Xiaocheng wrote: > dmazzoni@: Is it that AX is depending on incorrect behavior of TextIterator? > > When with first-letter, AXInlineTextBox seems to hold only the text box of > first-letter. Before this patch, TextIterator cannot correctly stop inside > first-letter, so it emits "Dt" even if the given range is just "D" (the > first-letter). > > With the patch, TextIterator only emits "D" when the given range is the > first-letter. However, it makes the AX tree dump look wrong... Happy to have this fixed!
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Slightly updated. Dead code related to trailing space collapsing will be done by another patch. https://codereview.chromium.org/2883163002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2883163002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:540: SpliceBuffer(kSpaceCharacter, text_node, 0, offset_, offset_); I don't think this line is reachable. I'll remove it in another patch. Same applies to a similar line in HandleReplacedElement().
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by yosin@chromium.org
The CQ bit was checked by yosin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2883163002/#ps40001 (title: "Tue May 16 16:31:00 PDT 2017")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaochengh@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaochengh@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495003255549670,
"parent_rev": "53265acf1e4bb0c0309315334589bd618c216cc0", "commit_rev":
"d91d54ced7ed0c79f8140e232f4b6b80efb5921d"}
Message was sent while issue was closed.
Description was changed from ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way: 1. Fix the logic when TextIterator decides: - Whether text in :first-letter should be emitted; Existing implementation emits :first-letter only when |offset_ == 0|, which is wrong if the iteration starts in :first-letter.. - Whether the iteration should end in :first-letter; Existing implementation has no handling of this at all. 2. When advancing from a text node's :first-letter to remaining text, |offset_| should be set to the starting offset of the remaining text, instead of 0. 3. Fix the logic when dealing with DOM and layout offsets in a text node with :first-letter. When a text node has :first-letter, offsets in DOM and layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox() should be aware of and handle the difference at all time; Existing implementation has no handling of the difference at all. New unit tests have also been added for the correct behavior. This patch is also a preparation for abstracting the text box handling logic out of TextIterator. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG=721957 TEST=TextIteratorTest.* ========== to ========== Un-insanify first-letter handling in TextIterator (to some degree) Background: (Once upon a time) TextIterator used to work on :first-letter in some cases, with several flawed designs magically cancelling each other's flaw. Recently, [1] fixed some flaw in TextIteratorTextState. However, it also built another level of hack over all the existing flawed designed in TextIterator, so that the class works in more cases. This patch removes the hack added by [1] and makes TextIterator work with :first-letter in an understandable way: 1. Fix the logic when TextIterator decides: - Whether text in :first-letter should be emitted; Existing implementation emits :first-letter only when |offset_ == 0|, which is wrong if the iteration starts in :first-letter.. - Whether the iteration should end in :first-letter; Existing implementation has no handling of this at all. 2. When advancing from a text node's :first-letter to remaining text, |offset_| should be set to the starting offset of the remaining text, instead of 0. 3. Fix the logic when dealing with DOM and layout offsets in a text node with :first-letter. When a text node has :first-letter, offsets in DOM and layout object differ by |TextStartOffset()|. HandleTextNode() and HandleTextBox() should be aware of and handle the difference at all time; Existing implementation has no handling of the difference at all. New unit tests have also been added for the correct behavior. This patch is also a preparation for abstracting the text box handling logic out of TextIterator. [1] crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f BUG=721957 TEST=TextIteratorTest.* Review-Url: https://codereview.chromium.org/2883163002 Cr-Commit-Position: refs/heads/master@{#472368} Committed: https://chromium.googlesource.com/chromium/src/+/d91d54ced7ed0c79f8140e232f4b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d91d54ced7ed0c79f8140e232f4b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
