New issue
Advanced search Search tips

Issue 877127 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

"Paste and match style" in Gmail moves cursor to beginning of paragraph

Project Member Reported by kbr@chromium.org, Aug 23

Issue description

Chrome 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.

 
Cc: vamshi.kommuri@chromium.org
Labels: Needs-Triage-M70 Triaged-ET Needs-Feedback
Unable to reproduce the issue on reported chrome version 70.0.3520.0 using Mac 10.13.1 with the below mentioned steps.
1. Launched Chrome & opened Gmail
2. Started composing a mail
3. Copied text from other application and pasted in mail
Observed the caret is at the end of the pasted text. Attaching the screen cast of the same for reference.

@kbr: Could you please have a look at the screen cast and let us know if we have missed anything in the process. Any further inputs from your end may be helpful.

Thanks!
877127.mp4
2.8 MB View Download
Labels: -Needs-Feedback
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).

Labels: Needs-Feedback
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!
877127-II.mp4
1.4 MB View Download
Status: WontFix (was: Untriaged)
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.

Status: Untriaged (was: WontFix)
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.

Owner: yoichio@chromium.org
Cc: yoichio@chromium.org
Owner: kbr@chromium.org
Not able to repro in Version 71.0.3561.0 (Official Build) canary (64-bit)
on mac.
Status: Unconfirmed (was: Untriaged)
Project Member

Comment 9 by sheriffbot@chromium.org, Sep 26

Labels: -Needs-Feedback
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
Summary: NEEDS_FEEDBACK "Paste and match style" in Gmail moves cursor to beginning of paragraph (was: "Paste and match style" in Gmail moves cursor to beginning of paragraph )
Labels: Needs-Feedback
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!
[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!
Summary: "Paste and match style" in Gmail moves cursor to beginning of paragraph (was: NEEDS_FEEDBACK "Paste and match style" in Gmail moves cursor to beginning of paragraph )
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

Project Member

Comment 14 by sheriffbot@chromium.org, Oct 1

Labels: -Needs-Feedback
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
I can repro in 71.0.3564.0 (Official Build) canary (64-bit)

It doesn't happen in 69.
Owner: ----
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.

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

Labels: Needs-Feedback
Owner: kbr@chromium.org
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).
Owner: yoichio@chromium.org
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.

Project Member

Comment 20 by sheriffbot@chromium.org, Oct 3

Labels: -Needs-Feedback
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
QA team, could you do bisect to identify which build causes this.
Thanks!
I could reproduce with simple contenteditable:
data:text/html,<div contenteditable></div>
on 71.0.3569.0 (Official Build) canary (64-bit)
Note: in steps #c19, double clicking on "bar" doesn't put CR nor LF to clipboard, it just put "bar" into clipboard.
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.



Owner: yosin@chromium.org
Status: Started (was: Unconfirmed)
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


 
Labels: -Needs-Bisect hasbisect-per-revision RegressedIn-70 Target-70 Target-71 FoundIn-71 FoundIn-70
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!
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().







In review: http://crrev.com/c/1260126
yosin@, just wondering are there any latest updates on the above change?
Project Member

Comment 30 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: Merge-TBD
[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.
How safe is this fix? Yosin@ is this safe enough to be merged to M70?
Labels: -Merge-TBD Merge-Request-70
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.
Labels: -Merge-Request-70 Merge-Approved-70
Project Member

Comment 36 by bugdroid1@chromium.org, Oct 15

Labels: -merge-approved-70 merge-merged-3538
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

Labels: Merge-Merged-70-3538
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