VisiblePositionInFlatTree constructor doesn't handle BeforeChildren well
Reported by
yfulgaon...@etouch.net,
Feb 23 2017
|
|||||||||
Issue descriptionChrome Version : 58.0.3020.0 (Official Build) 22a030be0c75c7a39b938d51d04bcc259bf9fe17-refs/heads/master@{#451874} 32/64 bit OS : Windows (7,8,10), Mac (10.11.6, 10.12.1, 10.12), Linux (14.04 LTS) What steps will reproduce the problem? 1. Launch chrome and navigate to chrome://settings page. 2. Click anywhere on the page and press 'Ctrl' + 'A' to select the text, observe. Actual : Text on the settings page does not get selected on pressing 'Ctrl' + 'A' keys. Expected : User should be able to select text using 'Ctrl' + 'A' on chrome://settings page. This is a regression issue broken in ‘M-58’, below is the Manual Regression range and will soon update other info. Good build : 58.0.3012.0 Bad build : 58.0.3013.0 Note : In step 2, after pressing 'Ctrl' + 'A' keys, only the 'ghost text' present inside 'Search settings' field gets selected.
,
Feb 24 2017
,
Feb 27 2017
See https://jsfiddle.net/hyes5orn/1/ When we put text before INPUT, selection is painted.
,
Feb 27 2017
Selection#selectAllChildren() works fine: https://jsfiddle.net/hyes5orn/2/ "selectAll" command: BODY@1 to BODY@6 (due by visible canonicalization) "selectAllChildren": BODY@0 to BODY@9
,
Feb 27 2017
The root cause is VisiblePositonInFlatTree with BeforeChildren position.
In <html><body><input></body><html>, VisiblePositionInFlatTree({HTML,BeforeChildren}) returns {INPUT,0}
,
Feb 27 2017
Thanks yosin@ for the update. Just to update, M-58 gets branched in 3 days from now. Would be good to have this fixed before branch point.
,
Feb 27 2017
In review: http://crrev.com/2718893003
,
Mar 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff3ad9a544321a740a585329d624ad78b4c6c551 commit ff3ad9a544321a740a585329d624ad78b4c6c551 Author: yosin <yosin@chromium.org> Date: Wed Mar 01 09:23:54 2017 Introduce PositionIteratorTest in webkit_unit_tests This patch introduces |PositionIteratorTest| to record current behavior to catch regression for improve code health. This patch is also for preparation of patch[1]. Note: |PositionIteratorTest| needs to have more test cases. This patch introduces test cases for patch[1]. [1] http://crrev.com/2720593005: Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA BUG= 695317 TEST=n/a; no behavior changes Review-Url: https://codereview.chromium.org/2725803002 Cr-Commit-Position: refs/heads/master@{#453893} [modify] https://crrev.com/ff3ad9a544321a740a585329d624ad78b4c6c551/third_party/WebKit/Source/core/editing/BUILD.gn [modify] https://crrev.com/ff3ad9a544321a740a585329d624ad78b4c6c551/third_party/WebKit/Source/core/editing/PositionIterator.cpp [modify] https://crrev.com/ff3ad9a544321a740a585329d624ad78b4c6c551/third_party/WebKit/Source/core/editing/PositionIterator.h [add] https://crrev.com/ff3ad9a544321a740a585329d624ad78b4c6c551/third_party/WebKit/Source/core/editing/PositionIteratorTest.cpp
,
Mar 2 2017
Just to update, Issue is still seen with the jsfiddle(https://jsfiddle.net/hyes5orn/1/) from C#3 as per the testing on Windows-10, Linux Ubuntu 14.04 on chrome version: 58.0.3028.0. Looks like there are few other patches to follow. This is marked as Beta blocker and M-58 gets branched on 03/02 PST. Requesting to please review the blocker label and adjust to Stable or if possible, land a fix before branch point. Thanks in advance!
,
Mar 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2211504ac2b999a125b2215ce7f6be9e50878fea commit 2211504ac2b999a125b2215ce7f6be9e50878fea Author: yosin <yosin@chromium.org> Date: Thu Mar 02 12:28:15 2017 Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to |PositionIterator| to make |canonicalizePositionOf()| not to return a position inside user agent shadow tree, e.g. a position of inner editor for INPUT to allow "SelectAll" command works correctly documents which start with INPUT, SELECT, and TEXTAREA element. Before this patch, if document starts with INPUT, "SelectAll" command selects contents in inner editor because flat tree version of |PositionIterator| returns such position for a position before INPUT. Then |VisibleSelection| shrink selection into inner editor of INPUT to avoid crossing editing boundary. This patch uses "user-select:contain"[2] idea for skipping contents of INPUT, SELECT and TEXTAREA, to align with the spec[2] rather than using internal term "selection boundary". [1] "empty-document-stylewithcss.html" is failed if we don't change |mostBackwardCaretPosition()| [2] https://drafts.csswg.org/css-ui-4/#user-interaction BUG= 695317 TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.* TEST=webkit_unit_tests --gtest_filter=VisibleSelectionTest.SelectAllWithInputElement TEST=webkit_unit_tests --gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement Review-Url: https://codereview.chromium.org/2720593005 Cr-Commit-Position: refs/heads/master@{#454236} [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/EditingUtilities.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/EditingUtilities.h [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/PositionIterator.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/PositionIteratorTest.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/VisibleSelectionTest.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/VisibleUnits.cpp [modify] https://crrev.com/2211504ac2b999a125b2215ce7f6be9e50878fea/third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp
,
Mar 3 2017
Verified the fix on the latest M-58(58.0.3029.0), this is working as intended as per the jsfiddle: https://jsfiddle.net/hyes5orn/1/ Adding the verified label and requesting yosin@ to please close the issue if no further work to be done here.
,
Mar 3 2017
,
Mar 3 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-58; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-58 label, otherwise remove Merge-TBD label. Thanks.
,
Mar 3 2017
This is already in M58 branch 3029, no merge is needed. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by msrchandra@chromium.org
, Feb 23 2017Owner: yosin@chromium.org
Status: Assigned (was: Unconfirmed)