Issue metadata
Sign in to add a comment
|
document.execCommand("insertOrderedList") is misbehaving |
||||||||||||||||||||||
Issue descriptionThis is b/121303599 internally and is occurring on 72.0.3626.28 (Official Build) beta (64-bit) To reproduce in Gmail: 1. Go to general settings, Place cursor in signature text box. 2. Type some text (If signature is not set already) and click on bulleted list/numbered list. I've dug in from our side, and it looks like we're just calling document.execCommand("insertOrderedList") But for whatever reason, Chrome is now making a copy of the DOM a few levels up and inserting it to the <li> elements.
,
Jan 5
Bisection result: You are probably looking for a change made after 600341 (known good), but no later than 600342 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/5707fca76b36b8801cbb004196a29ccd4675276d..5ebd2e2cb406f43178f10dfeea079b6d3a0cb7a4 Suspected culprit: crrev.com/c/1216282 Use MoveParagraphWithClones when moving paragraphs to new list items by Javier Fernandez jfernandez@: Could you take a look?
,
Jan 5
I'll take a loop ASAP
,
Jan 7
Since this is a regression, can I assume that this will make the release cut?
,
Jan 8
,
Jan 9
I'm working on this issue now.
,
Jan 9
Tentative fix for this issue: https://chromium-review.googlesource.com/c/chromium/src/+/1404256
,
Jan 9
Gmail team would strongly prefer this issue is fixed in M72. Is it possible to rollback the change that introduced this issue, rather than submitting a forward fix?
,
Jan 10
Request to merge into M72. The patch is ready to commit, thank jfernandez@!
,
Jan 10
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 10
> Gmail team would strongly prefer this issue is fixed in M72. Is it possible to rollback the change that introduced this issue, rather than submitting a forward fix? The proposed fix is pretty simple and I think safe to me merged in M72. If we find new cases that regressed and are not solved by this patch it wouldn't be difficult to rollback both, but I think it's worth to give this fix a try and merge it in M72.
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64b55aa53b5ed290694431631767b3cf92030108 commit 64b55aa53b5ed290694431631767b3cf92030108 Author: Javier Fernandez <jfernandez@igalia.com> Date: Thu Jan 10 08:51:19 2019 The outer node can't cross editing boundaries on paragraphs cloning Since r600342 we use the MoveParagraphWithClones function when we want to create list items from text paragraphs preserving their style. This change created a regression, as it's described in the bug this CL tries to solve. The problem is that a containing span outside the editing boundaries could be selected as outer node passed to the MoveParagraphWithClones function. This outer node can be used to define the node tree to clone. This outer node outside the editing boundaries may lead to duplicate wrongly some parts of the DOM tree. In order to fix the regression, we must ensure the outer node can't cross the editing boundaries. Bug: 919217 Change-Id: I2b60a63f8e451f48af407f67d1cb0883a77b4a6d Reviewed-on: https://chromium-review.googlesource.com/c/1404256 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#621506} [modify] https://crrev.com/64b55aa53b5ed290694431631767b3cf92030108/third_party/blink/renderer/core/editing/commands/insert_list_command.cc [modify] https://crrev.com/64b55aa53b5ed290694431631767b3cf92030108/third_party/blink/web_tests/editing/execCommand/listify-styled-paragraph.html
,
Jan 10
I'd need manual review for this merge request into M72
,
Jan 10
Pls apply appropriate OSs label and update bug with canary result tomorrow.
,
Jan 10
,
Jan 11
The NextAction date has arrived: 2019-01-11
,
Jan 11
,
Jan 11
abdulsyed@ for M72 merge review as this bug is more applicable to Desktop.
,
Jan 11
Doesn't apply to iOS so removing myself.
,
Jan 11
Approving this merge for M72. Branch:3626
,
Jan 11
CL with the cherry-pick waiting for review: https://chromium-review.googlesource.com/c/chromium/src/+/1407202
,
Jan 14
@yosin, could you please review the CL with the cherry-pick ?
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd178b92d23aae9d1ff86018e3190d980aaf341e commit bd178b92d23aae9d1ff86018e3190d980aaf341e Author: Javier Fernandez <jfernandez@igalia.com> Date: Mon Jan 14 08:58:37 2019 The outer node can't cross editing boundaries on paragraphs cloning Since r600342 we use the MoveParagraphWithClones function when we want to create list items from text paragraphs preserving their style. This change created a regression, as it's described in the bug this CL tries to solve. The problem is that a containing span outside the editing boundaries could be selected as outer node passed to the MoveParagraphWithClones function. This outer node can be used to define the node tree to clone. This outer node outside the editing boundaries may lead to duplicate wrongly some parts of the DOM tree. In order to fix the regression, we must ensure the outer node can't cross the editing boundaries. Bug: 919217 Change-Id: I2b60a63f8e451f48af407f67d1cb0883a77b4a6d Reviewed-on: https://chromium-review.googlesource.com/c/1404256 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#621506}(cherry picked from commit 64b55aa53b5ed290694431631767b3cf92030108) Reviewed-on: https://chromium-review.googlesource.com/c/1407202 Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Cr-Commit-Position: refs/branch-heads/3626@{#660} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/bd178b92d23aae9d1ff86018e3190d980aaf341e/third_party/blink/renderer/core/editing/commands/insert_list_command.cc [modify] https://crrev.com/bd178b92d23aae9d1ff86018e3190d980aaf341e/third_party/blink/web_tests/editing/execCommand/listify-styled-paragraph.html
,
Jan 14
I already merged it into Branch:3626
,
Jan 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd178b92d23aae9d1ff86018e3190d980aaf341e Commit: bd178b92d23aae9d1ff86018e3190d980aaf341e Author: jfernandez@igalia.com Commiter: jfernandez@igalia.com Date: 2019-01-14 08:58:37 +0000 UTC The outer node can't cross editing boundaries on paragraphs cloning Since r600342 we use the MoveParagraphWithClones function when we want to create list items from text paragraphs preserving their style. This change created a regression, as it's described in the bug this CL tries to solve. The problem is that a containing span outside the editing boundaries could be selected as outer node passed to the MoveParagraphWithClones function. This outer node can be used to define the node tree to clone. This outer node outside the editing boundaries may lead to duplicate wrongly some parts of the DOM tree. In order to fix the regression, we must ensure the outer node can't cross the editing boundaries. Bug: 919217 Change-Id: I2b60a63f8e451f48af407f67d1cb0883a77b4a6d Reviewed-on: https://chromium-review.googlesource.com/c/1404256 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#621506}(cherry picked from commit 64b55aa53b5ed290694431631767b3cf92030108) Reviewed-on: https://chromium-review.googlesource.com/c/1407202 Reviewed-by: Javier Fernandez <jfernandez@igalia.com> Cr-Commit-Position: refs/branch-heads/3626@{#660} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by xiaoche...@chromium.org
, Jan 5Status: Available (was: Untriaged)
Minimized repro in Blink: <span><div contenteditable="true">something</div></span> Set caret in "something" and then run 'insertOrderedList' command. The result HTML becomes: <span><div contenteditable="true"><ol><li><span><div contenteditable="true">something</div></span></li></ol></div></span> A closer repro of what's happening in GMail: <span> <table> <tbody> <tr> <td> <div contenteditable="true">something</div> </td> </tr> </tbody> </table> </span> In this case we get everything messed up, and "something" appears twice It seems that as long as there's a <span> somewhere containing the editable div, the command copies everything in the <span> into the <li> =================================== So there is a workaround: remove that containing <span> =================================== This is also a regression introduced in M72. Bisecting...