Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(26)

Issue 165083002: Issue 318925: Copy and paste sometimes removes spaces between words (Closed)

Created:
6 years, 10 months ago by c.shu
Modified:
6 years, 8 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

The problem is StyledMarkupAccumulator uses renderedText and the space at the end of the text has been stripped when the tag after the text was wrapped. BUG=318925 In the DOM tree, the copied content is represented by two nodes, one is "Copy this text " and the 2nd one is the anchor. When the code tries to convert the 1st node to plain text, it uses a text iterator to scan through the text. However, the spaces at the ending of the 1st node were lost because they were collapsed during rendering. This patch restores the missing space if it detects there is a non-text node following the last text node. This information has to be passed into the textIterator when it was constructed in StyledMarkupAccumulator::renderedText.

Patch Set 1 #

Patch Set 2 : Issue 318925: Copy and paste sometimes removes spaces between words #

Total comments: 3

Patch Set 3 : Issue 318925: Copy and paste sometimes removes spaces between words #

Patch Set 4 : Issue 318925: Copy and paste sometimes removes spaces between words #

Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -9 lines) Patch
A LayoutTests/editing/pasteboard/copy-text-with-wrapped-tag.html View 1 chunk +28 lines, -0 lines 0 comments Download
A LayoutTests/editing/pasteboard/copy-text-with-wrapped-tag-expected.txt View 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/editing/TextIterator.h View 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/editing/TextIterator.cpp View 1 2 3 2 chunks +6 lines, -2 lines 0 comments Download
M Source/core/editing/markup.cpp View 1 2 1 chunk +11 lines, -6 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
c.shu
Would you review this patch? Thanks!
6 years, 10 months ago (2014-02-13 22:59:23 UTC) #1
tyoshino (SeeGerritForStatus)
Looks like the script failed to upload base file for the Source/ dir. I can't ...
6 years, 10 months ago (2014-02-14 02:31:44 UTC) #2
c.shu
On 2014/02/14 02:31:44, tyoshino wrote: > Looks like the script failed to upload base file ...
6 years, 10 months ago (2014-02-14 02:48:12 UTC) #3
tyoshino (SeeGerritForStatus)
Thanks! I can also take some look but am not expert about this. Adding yosin@
6 years, 10 months ago (2014-02-14 04:38:41 UTC) #4
c.shu
On 2014/02/14 04:38:41, tyoshino wrote: > Thanks! I can also take some look but am ...
6 years, 10 months ago (2014-02-14 14:26:08 UTC) #5
c.shu
On 2014/02/14 14:26:08, c.shu wrote: > On 2014/02/14 04:38:41, tyoshino wrote: > > Thanks! I ...
6 years, 10 months ago (2014-02-14 15:24:29 UTC) #6
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/165083002/diff/180001/Source/core/editing/markup.cpp File Source/core/editing/markup.cpp (right): https://codereview.chromium.org/165083002/diff/180001/Source/core/editing/markup.cpp#newcode251 Source/core/editing/markup.cpp:251: TextIteratorBehavior behavior = TextIteratorDefaultBehavior; Move to right above L254? ...
6 years, 10 months ago (2014-02-17 09:20:50 UTC) #7
c.shu
I have addressed the review comments in the new patch. Thanks for the review!
6 years, 10 months ago (2014-02-18 15:36:26 UTC) #8
tyoshino (SeeGerritForStatus)
Thanks. Adding yutak@ as he's back now from vacation and should be an expert of ...
6 years, 10 months ago (2014-02-19 04:29:16 UTC) #9
Yuta Kitamura
As far as I can tell this patch won't work as expected. I created a ...
6 years, 10 months ago (2014-02-19 08:41:04 UTC) #10
c.shu
On 2014/02/19 08:41:04, Yuta Kitamura wrote: > As far as I can tell this patch ...
6 years, 10 months ago (2014-02-19 16:05:34 UTC) #11
c.shu
New patch addressed the review.
6 years, 10 months ago (2014-02-19 16:05:51 UTC) #12
Yuta Kitamura
On 2014/02/19 16:05:51, c.shu wrote: > New patch addressed the review. Your new patch still ...
6 years, 10 months ago (2014-02-20 02:51:38 UTC) #13
c.shu
> Your new patch still doesn't work on http://jsfiddle.net/d9EyK/2/ > (<span> </span> instead of a ...
6 years, 10 months ago (2014-02-20 15:17:25 UTC) #14
Yuta Kitamura
On 2014/02/20 15:17:25, c.shu wrote: > > Your new patch still doesn't work on http://jsfiddle.net/d9EyK/2/ ...
6 years, 10 months ago (2014-02-21 03:46:48 UTC) #15
c.shu
> I'm pretty sure that there *is* actually a space in the span, and > ...
6 years, 10 months ago (2014-02-21 16:55:58 UTC) #16
c.shu
> As I said before, I still don't think your patch is correct. > I ...
6 years, 10 months ago (2014-02-22 00:57:27 UTC) #17
Yuta Kitamura
On 2014/02/22 00:57:27, c.shu wrote: > > As I said before, I still don't think ...
6 years, 10 months ago (2014-02-25 02:44:05 UTC) #18
c.shu
> Um, I thought about this a lot and I still think this > change ...
6 years, 9 months ago (2014-03-03 14:10:48 UTC) #19
Yuta Kitamura
On 2014/03/03 14:10:48, c.shu wrote: > Do you have any details about handling collapsed spaces ...
6 years, 9 months ago (2014-03-05 07:16:39 UTC) #20
c.shu
6 years, 9 months ago (2014-03-05 14:07:38 UTC) #21
Here is what I think about the patch. Please check if it makes sense.
Based on the line numbers from this commit:
https://chromium.googlesource.com/chromium/blink/+/d8d116e1aa58f72ba8cb501820...,
from line 657-686, the code tries to emit text first, then detects if it's a
collapsed space case. This is Ok in the existing code because the flag is used
later when the next text node is scanned (see L556 for preformat case and L631
for regular cases). When the current node is the last one, no action will be
done even if the flag is true.
However, in our case, where we need to put one space at the end of the current
emission, we either emit the space at line 670 (this is what my patch does), or
redo the emission at line 681 when the collapsed space is determined. I have
tried the 2nd approach but there are other cases where spaces are collapsed but
do not fall into our case. We have to add extra if-else statement to single out
our case, which makes the code messy. In addition, it looks to me the rule is to
emit text only once in one run. We can manage to call emitText again but it's
confusing.
Another approach is to set the flag before emitting the text. Then in my change,
I just need to check the flag rather than checking str length, etc. I can try
this if you prefer.
Regarding to other cases where this patch cannot fix, what we found is the
<span> </span>. I figured out this falls into the case in line 593. And we have
to handle space restoration over there. I feel this is separate issue and I will
address it in another bug.

Thanks.

> What I'm worrying about this patch is, I guess, whether
> this patch is making things worse or not; to paraphrase,
> I suspect that this patch is just hiding the real issue,
> making the issue less reproducible without fixing
> the real problem behind the code. This is bad and will
> harm us in the long term. That's why I tried some
> different inputs as sanity checks (and as far as I know
> your patch failed this test).
> 
> If you still believe I'm wrong and your diff must be
> applied on that exact line, then you probably need to
> help me understand why you think so, in a logical manner.
> It wasn't obvious to me.

Powered by Google App Engine
This is Rietveld 408576698