|
|
Created:
3 years, 8 months ago by amaralp Modified:
3 years, 8 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing select all for contenteditable
Don't enable select all if the contenteditable has nothing to select.
BUG=696134
Review-Url: https://codereview.chromium.org/2787893004
Cr-Commit-Position: refs/heads/master@{#462762}
Committed: https://chromium.googlesource.com/chromium/src/+/099fc50c9790b25cf3ec7bad7aed7b75a8e98c80
Patch Set 1 #Patch Set 2 : forgot end quote #Patch Set 3 : format #Patch Set 4 : check if root editable node has children #Patch Set 5 : rebase #
Total comments: 4
Patch Set 6 : yosin's comments #
Messages
Total messages: 38 (27 generated)
The CQ bit was checked by amaralp@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 amaralp@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 amaralp@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Fixing select all for contenteditable BUG= ========== to ========== Fixing select all for contenteditable Don't enable select all if the contenteditable has nothing to select. BUG= 696134 ==========
amaralp@chromium.org changed reviewers: + yosin@chromium.org
PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it.
On 2017/04/04 at 16:16:31, amaralp wrote: > PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it. Could you analyze the failure test "text-selection-editing-crash.html"? It seems this change should not affect the result of this test. One observation of this change Ctrl+A doesn't set selection inside DIV in following case: // document.designMode = 'on' <div></div> // only markup In "text-selection-editing-crash.html", which execCommand replaces "Test passes if it does not CRASH.".
On 2017/04/05 at 02:12:02, yosin wrote: > On 2017/04/04 at 16:16:31, amaralp wrote: > > PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it. > > Could you analyze the failure test "text-selection-editing-crash.html"? > It seems this change should not affect the result of this test. > I think the problem is that the last 2 execCommand("SelectAll") aren't being run because |enabledSelectAll()| returns false for them. Also shouldn't selectAll be idempotent? This test implies that it isn't since select all is executed twice in a row. > One observation of this change Ctrl+A doesn't set selection inside DIV in following case: > // document.designMode = 'on' > <div></div> // only markup > I tried this on Stable Chrome on my linux workstation and |window.getSelection()| had the selection in the BODY not the DIV. > In "text-selection-editing-crash.html", which execCommand replaces "Test passes if it does not CRASH.". In my patch the first InsertHTML replaces it: document.execCommand("InsertHTML", false, "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>"). My theory for the sequence of events without my patch is: 1) First selectAll selects all text 2) Second selectAll deselects text and instead has a caret selection before "Test passes if it does not CRASH." 3) Formatblock works as expected 4) InsertHTML inserts "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>" before "Test passes if it does not CRASH." 5) InsertText inserts "xxxxxxxxxxxxxxxxxxx" right after the blockquote 6) FindString selects the "d" in "blockquoted" 7) Third selectAll makes the selection be a caret at the beginning of all the text. So right before "xxxx xxxxxx xxx xx..." 8) forwardDelete deletes the "x" that is to the right of the caret selection leaving "xxx xxxxxx xxx xx..." I think the problem is that the 2nd and 3rd select all's aren't acting as I would expect a select all should act. This might be due to the fact that the test has incorrect HTML. The tag <div class="box"> doesn't have a closing tag.
On 2017/04/05 at 03:20:04, amaralp wrote: > On 2017/04/05 at 02:12:02, yosin wrote: > > On 2017/04/04 at 16:16:31, amaralp wrote: > > > PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it. > > > > Could you analyze the failure test "text-selection-editing-crash.html"? > > It seems this change should not affect the result of this test. > > > I think the problem is that the last 2 execCommand("SelectAll") aren't being run because |enabledSelectAll()| returns false for them. > Also shouldn't selectAll be idempotent? This test implies that it isn't since select all is executed twice in a row. > > > > One observation of this change Ctrl+A doesn't set selection inside DIV in following case: > > // document.designMode = 'on' > > <div></div> // only markup > > > I tried this on Stable Chrome on my linux workstation and |window.getSelection()| had the selection in the BODY not the DIV. > > > > In "text-selection-editing-crash.html", which execCommand replaces "Test passes if it does not CRASH.". > In my patch the first InsertHTML replaces it: document.execCommand("InsertHTML", false, "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>"). So, second selectAll select "Test passes if it does not CRASH" instead of placing caret? It seems just check whether root editable has children or not instead of checking visible selection isRange? > My theory for the sequence of events without my patch is: > 1) First selectAll selects all text > 2) Second selectAll deselects text and instead has a caret selection before "Test passes if it does not CRASH." > 3) Formatblock works as expected > 4) InsertHTML inserts "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>" before "Test passes if it does not CRASH." > 5) InsertText inserts "xxxxxxxxxxxxxxxxxxx" right after the blockquote > 6) FindString selects the "d" in "blockquoted" > 7) Third selectAll makes the selection be a caret at the beginning of all the text. So right before "xxxx xxxxxx xxx xx..." > 8) forwardDelete deletes the "x" that is to the right of the caret selection leaving "xxx xxxxxx xxx xx..." > > I think the problem is that the 2nd and 3rd select all's aren't acting as I would expect a select all should act. This might be due to the fact that the test has incorrect HTML. > The tag <div class="box"> doesn't have a closing tag. Closing tag is auto magically inserted by parser.
The CQ bit was checked by amaralp@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
On 2017/04/05 at 03:40:19, yosin wrote: > On 2017/04/05 at 03:20:04, amaralp wrote: > > On 2017/04/05 at 02:12:02, yosin wrote: > > > On 2017/04/04 at 16:16:31, amaralp wrote: > > > > PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it. > > > > > > Could you analyze the failure test "text-selection-editing-crash.html"? > > > It seems this change should not affect the result of this test. > > > > > I think the problem is that the last 2 execCommand("SelectAll") aren't being run because |enabledSelectAll()| returns false for them. > > Also shouldn't selectAll be idempotent? This test implies that it isn't since select all is executed twice in a row. > > > > > > > One observation of this change Ctrl+A doesn't set selection inside DIV in following case: > > > // document.designMode = 'on' > > > <div></div> // only markup > > > > > I tried this on Stable Chrome on my linux workstation and |window.getSelection()| had the selection in the BODY not the DIV. > > > > > > > In "text-selection-editing-crash.html", which execCommand replaces "Test passes if it does not CRASH.". > > In my patch the first InsertHTML replaces it: document.execCommand("InsertHTML", false, "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>"). > > So, second selectAll select "Test passes if it does not CRASH" instead of placing caret? The first selectAll selects "Test passes if it does not CRASH" and the second select all doesn't get executed because |enabledSelectAll()| returns false. If the second select all were to execute then it would place a caret. Since it doesn't execute, we still have "Test passes if it does not CRASH" selected after the second selectAll. > It seems just check whether root editable has children or not instead of checking visible selection isRange? > I just uploaded a patch that checks if there are any children. There is an issue with the page only containing: <div contenteditable> </div> will have selectAll enabled because the DIV has the child: #text "\n" Maybe this is the result we would want?
The CQ bit was checked by amaralp@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/04/05 at 08:06:45, amaralp wrote: > On 2017/04/05 at 03:40:19, yosin wrote: > > On 2017/04/05 at 03:20:04, amaralp wrote: > > > On 2017/04/05 at 02:12:02, yosin wrote: > > > > On 2017/04/04 at 16:16:31, amaralp wrote: > > > > > PTAL, there is a layout test failing but I think it is only checking for a crash so rebaselining should fix it. > > > > > > > > Could you analyze the failure test "text-selection-editing-crash.html"? > > > > It seems this change should not affect the result of this test. > > > > > > > I think the problem is that the last 2 execCommand("SelectAll") aren't being run because |enabledSelectAll()| returns false for them. > > > Also shouldn't selectAll be idempotent? This test implies that it isn't since select all is executed twice in a row. > > > > > > > > > > One observation of this change Ctrl+A doesn't set selection inside DIV in following case: > > > > // document.designMode = 'on' > > > > <div></div> // only markup > > > > > > > I tried this on Stable Chrome on my linux workstation and |window.getSelection()| had the selection in the BODY not the DIV. > > > > > > > > > > In "text-selection-editing-crash.html", which execCommand replaces "Test passes if it does not CRASH.". > > > In my patch the first InsertHTML replaces it: document.execCommand("InsertHTML", false, "<blockquote type='cite'>xxxx xxxxxx xxx xx blockquoted.</blockquote>"). > > > > So, second selectAll select "Test passes if it does not CRASH" instead of placing caret? > The first selectAll selects "Test passes if it does not CRASH" and the second select all doesn't get executed because |enabledSelectAll()| returns false. If the second > select all were to execute then it would place a caret. Since it doesn't execute, we still have "Test passes if it does not CRASH" selected after the second selectAll. > > > It seems just check whether root editable has children or not instead of checking visible selection isRange? > > > I just uploaded a patch that checks if there are any children. There is an issue with the page only containing: > > <div contenteditable> > </div> > > will have selectAll enabled because the DIV has the child: #text "\n" > Maybe this is the result we would want? Yes, we want to remove "\n" by "SelectAll" + "Delete". But current Blink doesn't allow this due by visible canonicalization. :-< I tried "selectAll" on content editable w/ collapsed whitespace only in https://jsfiddle.net/khezdnjp/2/ and results are: - Edge: Selection is DIV@0 to DIV@1 - Firefox: Selection is BODY@0 to BODY@7; it seems Firefox doesn't allow set focus to content editable w/o visible content - Chrome: DIV@0 to DIV0 I think Edge does right thing, it select all contents in content editable. We'll make Blink to work as Edge in later.
https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2118: Node* root = highestEditableRoot(selection.start()); For safety, we would like to do null check for |root| and |isContentEditable()| does same thing. So, we could write: if (Node* root = highedtEditableRoot(selection.start())) { if (!root->hasChildrent()) return false; } https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11584: EXPECT_TRUE(testSelectAll("<div contenteditable>nonempty</div>")); Could you add |EXPECT_TRUE(testSelectAll("<div contenteditable>\n</div>"));|?
The CQ bit was checked by amaralp@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.
https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:2118: Node* root = highestEditableRoot(selection.start()); On 2017/04/06 at 08:27:27, yosin_UTC9 wrote: > For safety, we would like to do null check for |root| and |isContentEditable()| does same thing. > So, we could write: > > if (Node* root = highedtEditableRoot(selection.start())) { > if (!root->hasChildrent()) > return false; > } Done. https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2787893004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11584: EXPECT_TRUE(testSelectAll("<div contenteditable>nonempty</div>")); On 2017/04/06 at 08:27:27, yosin_UTC9 wrote: > Could you add |EXPECT_TRUE(testSelectAll("<div contenteditable>\n</div>"));|? Done.
lgtm
The CQ bit was checked by amaralp@chromium.org
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": 100001, "attempt_start_ts": 1491541311819190, "parent_rev": "183b8cb430ef80e40afc4b4659f77f5832075b2d", "commit_rev": "099fc50c9790b25cf3ec7bad7aed7b75a8e98c80"}
Message was sent while issue was closed.
Description was changed from ========== Fixing select all for contenteditable Don't enable select all if the contenteditable has nothing to select. BUG= 696134 ========== to ========== Fixing select all for contenteditable Don't enable select all if the contenteditable has nothing to select. BUG= 696134 Review-Url: https://codereview.chromium.org/2787893004 Cr-Commit-Position: refs/heads/master@{#462762} Committed: https://chromium.googlesource.com/chromium/src/+/099fc50c9790b25cf3ec7bad7aed... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/099fc50c9790b25cf3ec7bad7aed... |