Issue metadata
Sign in to add a comment
|
"Paste and match style" in Gmail moves cursor to beginning of paragraph |
||||||||||||||||||||||||
Issue descriptionChrome Version: 70.0.3520.0 (Official Build) canary (64-bit) OS: macOS 10.13.6 What steps will reproduce the problem? (1) Compose mail in Gmail (2) Copy some text from another application to the clipboard (3) Start a paragraph (3) Paste and match style (Command-Shift-V) What is the expected result? Expect caret to be at the end of the pasted text. What happens instead? Caret is warped to the beginning of the paragraph. This is a regression in Chrome. The same actions work fine in 68.0.3440.106 (Official Build) (64-bit). This is a serious usability regression. Marking P1, ReleaseBlock-Stable. I first noticed this a couple of days ago but unfortunately didn't think to report it.
,
Aug 24
The paste operation needs to be done while the cursor is at the end of a line. For example, start typing: "This is a test " and then "Paste and match style". If the paste operation is done while the caret is at the beginning of a line it works properly. This is still reproducible in the current Canary on macOS, 70.0.3532.0 (Official Build) canary (64-bit).
,
Aug 27
Tried checking the issue on reported chrome version 70.0.3520.0 using Mac 10.13.1 by typing "This is a test" then pasting(Command + Option + Shift + V) the text copied on clip board. We didn't observe the cursor moving to the beginning of the paragraph. Attaching the screen cast of the same for reference. @kbr: Could you please have a look at the screencast and let us know if we have missed anything in the process. Thanks!
,
Aug 27
The problem seems to have disappeared over the weekend. With 70.0.3534.0 (Official Build) canary (64-bit) and the current version of Gmail (not sure how to get the version number anymore) this isn't reproducible any more.
,
Sep 18
Reopening. This problem is happening in 71.0.3552.2 (Official Build) canary (64-bit) on macOS. I think my mistake was specifying to select text from external applications rather than Chrome. Reproduction steps: 1) Compose a new Gmail message 2) Type or copy the following text into the body: (between the dotted lines) ----- Regression range 555400:555416 Regression range ----- 3) Select the text "555400:555416" and copy it. 4) Position the caret after the second "Regression range ". 5) Paste special (Cmd-Shift-V). The caret will warp to the beginning of the line.
,
Sep 25
,
Sep 25
Not able to repro in Version 71.0.3561.0 (Official Build) canary (64-bit) on mac.
,
Sep 26
,
Sep 26
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 26
,
Sep 27
Re-tested the issue on latest chrome version 71.0.3562.0 using Mac 10.13.1, still we couldn't reproduce the issue from our end too. Adding Needs-Feedback label and requesting kbr@ to confirm it's behaviour on latest canary. Thanks!
,
Oct 1
[bulk edit] - This issue is marked as a stable blocker for M70. We are two weeks away from M70 Stable. Please take a look urgently!
,
Oct 1
It's still happening with 71.0.3567.0 (Official Build) canary (64-bit). The reproduction steps from https://bugs.chromium.org/p/chromium/issues/detail?id=877127#c5 above work for me to reproduce the problem. The full contents of about:chrome follow, including all the experiments which are enabled on my profile. (--isolate-origins is abbreviated.) I haven't tried creating a new user-data-dir to see whether it's likely it's related to an experiment. Google Inc. Copyright 2018 Google Inc. All rights reserved. Google Chrome 71.0.3567.0 (Official Build) canary (64-bit) Revision 7c7bc73fc5f252e28f2308c81de4760e49990542-refs/branch-heads/3567@{#1} OS Mac OS X JavaScript V8 7.1.216 Flash 31.0.0.122 /Users/kbr/Library/Application Support/Google/Chrome Canary/PepperFlash/31.0.0.122/PepperFlashPlayer.plugin User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3567.0 Safari/537.36 Command Line /Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary --isolate-origins=https://chromium.org/,[...] --flag-switches-begin --memlog=manual --enable-features=WebAssembly --flag-switches-end Executable Path /Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary Profile Path /Users/kbr/Library/Application Support/Google/Chrome Canary/Default Variations 2c707b42-5940ee21 411b6d4e-3f4a17df b7d3b6c2-3f4a17df fe69e053-94941f92 d01ab0d3-614f7c0c 16e0dd70-3f4a17df 66df3e9d-a3a14831 b7e2524c-f23d1dea a6674cf-3fcacf88 da89714-4ad60575 64da5c1e-7bc2af31 61832c80-3f4a17df cc20827f-ca7d8d80 9041608a-f23d1dea 5852bcb0-a75ab0e c6c1fc26-3d47f4f4 9853922b-c200976c 6025934e-3f4a17df 7c1bc906-b5809d46 9def365c-f23d1dea 47e5d3db-3d47f4f4 125b7f68-65bced95 d442dfb7-ca7d8d80 9ca1387e-3f4a17df 1149accc-65bced95 6557d030-f23d1dea 34d450b1-6fa8045b a582a1b8-ad75ce17 495970ba-ca7d8d80 249dd49a-28165b59 ac6e1b9-d93a0620 aa011017-a2d707c6 44827ee5-43146c13 edbcf7c5-961c461c 5485fc4d-3f4a17df 93731dca-e89d496c e111fcd-f23d1dea 41f007f9-41f007f9 9b4c4257-592e7888 1b558915-3f4a17df 43f62d3b-ca7d8d80 4ae380f4-3f4a17df c992f345-ca7d8d80 9e5c75f1-e406a769 67fc9f6f-65bced95 350fabdd-34b13816 6fa07eb4-efa92713 4934552d-f23d1dea 2ca9c26b-3f4a17df 2b08b14-3f4a17df 7a5ba892-3f4a17df 4ea303a6-6fc10f6d 95876445-45e665ef d92562a9-65bced95 fc369826-3f4a17df 67246da1-f23d1dea de52c077-65bced95 cc54eb06-3f4a17df 58a025e3-36e97b2c ad6d27cc-15e2aa9a df072bba-9a6c5085 8576baf1-f23d1dea 23496387-232b3cab 2e7f6029-f23d1dea 1fcbb124-9aaad08b 344833e9-473e8c2e 4bc337ce-a1f4f53d 9a2f4e5b-3fe9c4dc 494d8760-52325d43 3ac60855-3ec2a267 f296190c-477de798 4442aae2-4ad60575 ed1d377-e1cc0f14 75f0f0a0-4ad60575 e2b18481-7158671e e7e71889-4ad60575 5e60f31d-1410f10 6a51bb09-ca7d8d80 b0ea13bc-3f4a17df 6b5f0f2d-1410f10 94e68624-f23d1dea cc73f8a1-a2d707c6 b4e8892d-f23d1dea 10a311eb-cf4f6ead 81c6897f-741d8d64 3a17127a-870290a7
,
Oct 1
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1
I can repro in 71.0.3564.0 (Official Build) canary (64-bit) It doesn't happen in 69.
,
Oct 1
Moving this to unowned again. I'm not sure why it's so difficult to reproduce; my teammate has reproduced it easily on a Mac laptop running Chrome 71. Maybe it's not R-B-S for 70 if nobody else is complaining about it.
,
Oct 3
Note: I could not reproduce this on Windows and Linux both Gmail composer and simple content editable. It seems editing/commands/ doesn't have suspicious changes: 9cd1162 by mustaq on 2018-10-03 01:30 (local) Moved UserActivation entry points to LocalFrame. c743581 by tkent@chromium.org on 2018-09-28 16:36 (local) DOM, Editing, HTML: Fix CamelCase file names in comments 3446c85 by tkent@chromium.org on 2018-09-28 13:19 (local) DOM, Editing, HTML: Correct m_fooBar in comments. 0bad678 by keishi@chromium.org on 2018-09-20 20:11 (local) Merge DISALLOW_NEW and DISALLOW_NEW_EXCEPT_PLACEMENT_NEW 483db09 by hs1217.lee@samsung.com on 2018-09-20 13:07 (local) Use Element& instead of Element* as a parameter of HasEquivalentAttributes 5b56452 by dtapuska@chromium.org on 2018-09-13 22:02 (local) Fix 64 bit truncation errors in blink/editing 8ed4529 by tkent@chromium.org on 2018-08-24 21:08 (local) DOM: Reduce the size of event.h 0936336 by yoichio@chromium.org on 2018-08-16 14:13 (local) Refactoring: let DocumentMarkerController::MoveMarkers only receive |const Text&|. eaa7252 by xiaochengh@chromium.org on 2018-08-15 11:06 (local) Abort formatBlock command when SelectionForParagraphIteration() errs e27bd82 by xiaochengh@chromium.org on 2018-08-14 13:51 (local) Abort formatBlock command when paragraph range calculation errs
,
Oct 3
Kenneth, could you take a movie? The word "copy" or "move a caret" is ambiguous because there are various ways to do that. Could you clarify each of them by mouse or by keyboard? > 1) Compose a new Gmail message That means clicking the Compose button on the right-top and start to edit on new message window w/o expanding it? > 2) Type or copy the following text into the body: (between the dotted lines) I don't know the dotted line. Could you take a screen shot? ----- Regression range 555400:555416 Regression range ----- > 3) Select the text "555400:555416" and copy it. > 4) Position the caret after the second "Regression range ". How did you do this? Mouse dragging and/or expanding it by shift-arrow? 5) Paste special (Cmd-Shift-V).
,
Oct 3
Here's a movie showing the bug (Google internal only): https://drive.google.com/file/d/1-8Emubvr7LPU5U_2d6QymcIL9bB8wphp/view?usp=sharing In creating it I realized what the bug is. If you double-click the word at the end of a line, then Command-C to copy, the carriage return character will be put on the clipboard at the end of the text. Then if you "paste special", the carriage return will be misinterpreted, and the caret will be moved to the beginning of the line. Reduced test case: 1) Click the compose button in Gmail. 2) In the message body, type the text: foo bar<enter><enter>baz . 3) Double click "bar" and copy (Command-C). 4) Click the mouse on the line after "baz". 5) Paste special (Command-V). The last line will now be "baz bar" but the cursor will be warped to the beginning of the line, which is the behavioral change and the regression.
,
Oct 3
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 4
QA team, could you do bisect to identify which build causes this. Thanks!
,
Oct 4
I could reproduce with simple contenteditable: data:text/html,<div contenteditable></div> on 71.0.3569.0 (Official Build) canary (64-bit)
,
Oct 4
Note: in steps #c19, double clicking on "bar" doesn't put CR nor LF to clipboard, it just put "bar" into clipboard.
,
Oct 4
I also managed in m71 canary. 1. go "data:text/html,<div contenteditable>foo bar<br><div>baz<div></div>" 2. double click "bar" and push "Cmd-c" to copy it. 3. Put a cursor to the end of baz by click(I guess by keyboard also causes). 4. Push "Cmd-Shift-v" to paste "bar" You see the cursor is at the start of the second line. foo bar '|'baz bar Also I wonder why a space is inserted before pasted "bar". It also was caused by Cmd-v w/o moving cursor to the start.
,
Oct 4
I found root cause. In unit test[1], I hit DCHECK at Position(text, offset) constructor, where offset is out of bound.
This is caused in
void ReplaceSelectionCommand::CompleteHTMLReplacement() {
...
if (match_style_) {
DCHECK(insertion_style_);
ApplyStyle(insertion_style_.Get(), start, end, editing_state);
if (editing_state->IsAborted())
return;
}
if (last_position_to_select.IsNotNull())
end = last_position_to_select;
MergeTextNodesAroundPosition(start, end, editing_state);
if (editing_state->IsAborted())
return;
...
Where ApplyStyle() makes |end| position to out of bound then we pass the invalid position |end| to MergeTextNodesAroundPosition().
This issue has been existing from beginning. Before M70, it seemed worked, but from M70, this unexpected behavior place caret at wrong place.
We still see empty Text node in content editable due by this issue.
[1] http://crrev.com/c/1260126
,
Oct 4
Bisected as per the steps mentioned in comment#24. Able to reproduce the issue on Mac 10.13.1 on reported chrome version 70.0.3520.0 and on latest canary 71.0.3570.0. Note: Issue isn't seen on Windows 10 and Ubuntu 14.04 Bisect Information: ------------------- Good Build: 70.0.3518.0 Bad Build: 70.0.3519.0 You are probably looking for a change made after 582089 (known good), but no later than 582090 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/021565ba51cfea2594e02bb67a367118daaf5021..673dd7ce01807a268ff27f3837cb402fe90a29e6 Suspecting: https://chromium.googlesource.com/chromium/src/+/673dd7ce01807a268ff27f3837cb402fe90a29e6 Review URL: https://chromium-review.googlesource.com/1170458 Thanks!
,
Oct 5
vamshi.kommuri@, thanks for bisecting! Suspecting CL you pointed out in #c26 is a criminal to move caret to start of the line in CorrectedSelectionAfterCommand(). ReplaceSelection command attempts to set selection with invalid position. We're fixing ReplaceSelectionCommand not to use invalid position, out-of-bound position in this case, and pass proper selection to CorrectedSelectionAfterCommand().
,
Oct 5
In review: http://crrev.com/c/1260126
,
Oct 8
yosin@, just wondering are there any latest updates on the above change?
,
Oct 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dda816d236f796571ef338fb380399baaddd9dd1 commit dda816d236f796571ef338fb380399baaddd9dd1 Author: Yoshifumi Inoue <yosin@chromium.org> Date: Tue Oct 09 10:19:03 2018 Make ReplaceSelectionCommand::CompleteHTMLReplacement() not to use out-of-bound position This patch changes |ReplaceSelectionCommand::CompleteHTMLReplacement()| not to use out-of-bound position to proper execution and avoid to hit DCHECK() in |Position| constructor. This issue is cause by positions hold in local variable |start| and |end| can be out-of-position by |ApplyStyle()| since it split |Text| node which is anchor node of |start| and |end|. Bug: 877127 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I564bbe953ab7660e099b193cb5b59e68d6454e8f Reviewed-on: https://chromium-review.googlesource.com/c/1260126 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoichi Osato <yoichio@chromium.org> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Commit-Position: refs/heads/master@{#597866} [modify] https://crrev.com/dda816d236f796571ef338fb380399baaddd9dd1/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc [modify] https://crrev.com/dda816d236f796571ef338fb380399baaddd9dd1/third_party/blink/renderer/core/editing/commands/replace_selection_command_test.cc
,
Oct 10
,
Oct 10
[Auto-generated comment by a script] We noticed that this issue is targeted for M-70; 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-70 label, otherwise remove Merge-TBD label. Thanks.
,
Oct 11
How safe is this fix? Yosin@ is this safe enough to be merged to M70?
,
Oct 15
Mark Merge-Request-M70 since fix should be fix and current behavior makes users confusing. >#c33, fix should be fix because of: - create new Range object - use it There are no chance to cause crash.
,
Oct 15
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7542c2e3942eeec520bacd531ed181c98773676 commit f7542c2e3942eeec520bacd531ed181c98773676 Author: Yoshifumi Inoue <yosin@chromium.org> Date: Mon Oct 15 02:23:39 2018 Make ReplaceSelectionCommand::CompleteHTMLReplacement() not to use out-of-bound position This patch changes |ReplaceSelectionCommand::CompleteHTMLReplacement()| not to use out-of-bound position to proper execution and avoid to hit DCHECK() in |Position| constructor. This issue is cause by positions hold in local variable |start| and |end| can be out-of-position by |ApplyStyle()| since it split |Text| node which is anchor node of |start| and |end|. Bug: 877127 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I564bbe953ab7660e099b193cb5b59e68d6454e8f Reviewed-on: https://chromium-review.googlesource.com/c/1260126 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoichi Osato <yoichio@chromium.org> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#597866}(cherry picked from commit dda816d236f796571ef338fb380399baaddd9dd1) Reviewed-on: https://chromium-review.googlesource.com/c/1278641 Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#997} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/f7542c2e3942eeec520bacd531ed181c98773676/third_party/blink/renderer/core/editing/commands/replace_selection_command.cc [modify] https://crrev.com/f7542c2e3942eeec520bacd531ed181c98773676/third_party/blink/renderer/core/editing/commands/replace_selection_command_test.cc
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7542c2e3942eeec520bacd531ed181c98773676 Commit: f7542c2e3942eeec520bacd531ed181c98773676 Author: yosin@chromium.org Commiter: yosin@chromium.org Date: 2018-10-15 02:23:39 +0000 UTC Make ReplaceSelectionCommand::CompleteHTMLReplacement() not to use out-of-bound position This patch changes |ReplaceSelectionCommand::CompleteHTMLReplacement()| not to use out-of-bound position to proper execution and avoid to hit DCHECK() in |Position| constructor. This issue is cause by positions hold in local variable |start| and |end| can be out-of-position by |ApplyStyle()| since it split |Text| node which is anchor node of |start| and |end|. Bug: 877127 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I564bbe953ab7660e099b193cb5b59e68d6454e8f Reviewed-on: https://chromium-review.googlesource.com/c/1260126 Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoichi Osato <yoichio@chromium.org> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#597866}(cherry picked from commit dda816d236f796571ef338fb380399baaddd9dd1) Reviewed-on: https://chromium-review.googlesource.com/c/1278641 Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#997} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by vamshi.kommuri@chromium.org
, Aug 24Labels: Needs-Triage-M70 Triaged-ET Needs-Feedback
2.8 MB
2.8 MB View Download