Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907963002:
[Smart Text] Make SurroundingText work for input elements (Closed)
|
Created:
3 years, 7 months ago by Tima Vaisburd Modified:
3 years, 6 months ago CC:
aelias_OOO_until_Jul13, blink-reviews, chromium-reviews, Donn Denman, Theresa Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Text] Make SurroundingText work for input elements
The existing algorithm sets the maximal range of forward and backward
character iterators and then advances them. The maximal range used to
be the whole document, in this CL we limit the range to be inside the
root editable element if such element exists.
BUG=721840
Review-Url: https://codereview.chromium.org/2907963002
Cr-Commit-Position: refs/heads/master@{#477572}
Committed: https://chromium.googlesource.com/chromium/src/+/48824066704f4c092f5193ae38f5c457d688813e
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 4
Patch Set 3 : Used ContainingShadowRoot() #Patch Set 4 : Using RootEditableElementOf(Position) #Patch Set 5 : Restored and updated a comment, improved the test. #
Total comments: 6
Patch Set 6 : Made pointers const #
Messages
Total messages: 38 (28 generated)
The CQ bit was checked by timav@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: This issue passed the CQ dry run.
timav@chromium.org changed reviewers: + amaralp@chromium.org, yosin@chromium.org
PTAL.
Could you add a test case for this change? Thanks!
The CQ bit was checked by timav@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...
On 2017/05/31 02:10:14, yosin_UTC9 wrote: > Could you add a test case for this change? > Thanks! Yes, that was more a question whether approach was right. Now added test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I don't think using tree scope boundaries is not good idea for computing surrounding text. How about using RootEditableElementOf(Position)? Since selection can't cross over editing boundaries, iterating root editable element can produce same result for INPUT/TEXTAREA as using shadow boundaries. Note: CSS property user-selection[1], not yet implemented, controls selectable range. At this time, contentEditable=true implies user-select:containment [1] https://drafts.csswg.org/css-ui-4/#propdef-user-select https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SurroundingText.cpp (right): https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SurroundingText.cpp:70: Node* end_root_container = end_position.ComputeContainerNode(); You can do this by Node* end_container = end_position.ComputeContianerNode(); Node* end_root_container = end_container->ContainingShadowRoot() ? end_container->ContainingShadowRoot() : document; For CharacterIterator, LastPositionInNode(document) and LastPositonInNode(document->documentElement()) are equivalent. We may have some anti-pattern, e.g. - document.documentElement.append('foo') - document.replace(documentElement, 'foo') makes document->documentElement() == nullptr https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SurroundingText.cpp:74: Node* last_node = end_root_container->IsShadowRoot() The name |last_node| is confusion. Usually, document element isn't the last node. Sorry, I don't have an idea of another name. Although, when we use Node::ContainingShadowRoot(), we don't need to use |last_node|.
The CQ bit was checked by timav@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: This issue passed the CQ dry run.
The CQ bit was checked by timav@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...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by timav@chromium.org
The CQ bit was checked by timav@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 ========== [Smart Text] Make SurroundingText work for input elements Keep the range of forward_iterator and backward_iterator limited to the root container node of the end and start position correspondingly. For the case of <input> and <textarea> tags the root container of the selected text is Shadow DOM, not the Document. This CL keeps the range of the iterators inside Shadow DOM in this case. BUG=721840 ========== to ========== [Smart Text] Make SurroundingText work for input elements The existing algorithm sets the maximal range of forward and backward character iterators and then advances them. The maximal range used to be the whole document, in this CL we limit the range to be inside the root editable element if such element exists. BUG=721840 ==========
https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SurroundingText.cpp (right): https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SurroundingText.cpp:70: Node* end_root_container = end_position.ComputeContainerNode(); On 2017/06/05 04:59:32, yosin_UTC9 wrote: > You can do this by > > Node* end_container = end_position.ComputeContianerNode(); > Node* end_root_container = end_container->ContainingShadowRoot() ? > end_container->ContainingShadowRoot() : document; I used RootEditableElementOf() eventually, I thought it is the preferred way. > > For CharacterIterator, LastPositionInNode(document) and > LastPositonInNode(document->documentElement()) are equivalent. Unfortunately many tests broke after I replaced document->documentElement() with document (off-by-one error in the ranges). https://codereview.chromium.org/2907963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SurroundingText.cpp:74: Node* last_node = end_root_container->IsShadowRoot() On 2017/06/05 04:59:32, yosin_UTC9 wrote: > The name |last_node| is confusion. Usually, document element isn't the last > node. > Sorry, I don't have an idea of another name. > > Although, when we use Node::ContainingShadowRoot(), we don't need to use > |last_node|. Not used anymore.
The CQ bit was checked by timav@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...
lgtm w/ nits https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SurroundingText.cpp (right): https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingText.cpp:71: Element* root_editable = RootEditableElementOf(start_position); nit: s/Element*/Element* const/ https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingText.cpp:72: Element* root_element = nit: s/Element*/Element* const/ https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp (right): https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp:7: #include <memory> Thanks for reorder to follow coding standard.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SurroundingText.cpp (right): https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingText.cpp:71: Element* root_editable = RootEditableElementOf(start_position); On 2017/06/07 01:00:00, yosin_UTC9 wrote: > nit: s/Element*/Element* const/ Done. https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingText.cpp:72: Element* root_element = On 2017/06/07 01:00:00, yosin_UTC9 wrote: > nit: s/Element*/Element* const/ Done. https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp (right): https://codereview.chromium.org/2907963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SurroundingTextTest.cpp:7: #include <memory> On 2017/06/07 01:00:00, yosin_UTC9 wrote: > Thanks for reorder to follow coding standard. Thank you, but can't take the credit: this was "git cl format".
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2907963002/#ps120001 (title: "Made pointers const")
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": 120001, "attempt_start_ts": 1496811176544360,
"parent_rev": "1be2d90f389c960e6e452d3e4f465d8659050e6e", "commit_rev":
"48824066704f4c092f5193ae38f5c457d688813e"}
Message was sent while issue was closed.
Description was changed from ========== [Smart Text] Make SurroundingText work for input elements The existing algorithm sets the maximal range of forward and backward character iterators and then advances them. The maximal range used to be the whole document, in this CL we limit the range to be inside the root editable element if such element exists. BUG=721840 ========== to ========== [Smart Text] Make SurroundingText work for input elements The existing algorithm sets the maximal range of forward and backward character iterators and then advances them. The maximal range used to be the whole document, in this CL we limit the range to be inside the root editable element if such element exists. BUG=721840 Review-Url: https://codereview.chromium.org/2907963002 Cr-Commit-Position: refs/heads/master@{#477572} Committed: https://chromium.googlesource.com/chromium/src/+/48824066704f4c092f5193ae38f5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/48824066704f4c092f5193ae38f5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
