Restore a collapsed trailing space of text used for line break

Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html

Committed: https://crrev.com/9b93a675c78452c27f1749ff243ab8f9449b1184
Cr-Commit-Position: refs/heads/master@{#416883}

[joone, 2016-08-25 23:31:12]

Patchset #1 (id:1) has been deleted

[joone, 2016-08-25 23:31:23]

Patchset #1 (id:20001) has been deleted

[joone, 2016-08-25 23:33:24]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 23:34:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-08-25 23:59:22]

Description was changed from

==========
Restore a collapsed trailing space of text when it is used for line break

When we copy a wrapped text, the trailing spaces of the text are
collapsed and a line break is insersted instead of it during layout.
In this case, we need to restore the collapsed space when we copy
the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

This CL allows Blink to set a flag in LayoutText when the line break happnes
during layout so it could handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text when it is used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL allows Blink to set a flag in LayoutText when the line break
happens during layout so it could handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[joone, 2016-08-26 00:01:07]

Description was changed from

==========
Restore a collapsed trailing space of text when it is used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL allows Blink to set a flag in LayoutText when the line break
happens during layout so it could handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL allows Blink to set a flag in LayoutText when the line break
happens during layout so it could handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[joone, 2016-08-26 00:09:40]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL allows Blink to set a flag in LayoutText when the line break
happens during layout so it could handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[joone, 2016-08-26 00:41:15]

joone.hur@intel.com changed reviewers:
+ esprehn@chromium.org, yosin@chromium.org

[joone, 2016-08-26 00:41:17]

Hi,

This is a better way of fixing http://crbug.com/318925, which handles more
cases. Please take a look at it.

Also, this CL removes the unintended effects:

https://codereview.chromium.org/2280513004/diff/40001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/2280513004/diff/40001/content/renderer/render...
content/renderer/render_view_browsertest.cc:1738: EXPECT_EQ(output, "hello
\n\nworld");
The previous fix(https://codereview.chromium.org/2193033004/) unintentionally
added a space so we need to remove it.

https://codereview.chromium.org/2280513004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/execCommand/script-tests/remove-format-multiple-elements-mac.js
(right):

https://codereview.chromium.org/2280513004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/execCommand/script-tests/remove-format-multiple-elements-mac.js:68:
testRemoveFormat('<b>hello <dfn>world <kbd>WebKit</kbd></dfn></b>',
selectSecondWord, '<b>hello </b>world<b><dfn> <kbd>WebKit</kbd></dfn></b>');
This seems to be unintentionally fixed by the previous CL because there is no
line break in this test case.

https://codereview.chromium.org/2280513004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/selection/caret-at-bidi-boundary-expected.txt
(left):

https://codereview.chromium.org/2280513004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/selection/caret-at-bidi-boundary-expected.txt:3:
Current offset:
The previous fix unintentionally added a space so we need to remove it.

[joone, 2016-08-26 00:42:59]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 00:43:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-08-26 00:45:02]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

The previous fix has the unintended effect that the trailing space
can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[joone, 2016-08-26 00:45:55]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[yosin_UTC9, 2016-08-26 01:55:44]

I'm not a fan of this patch because of introducing
LayoutText::needToRestoreCollapsedSpace().
I feel meaning of this API is vague.

I'm thinking of introducing LayoutPosition, http://crrev.com/1756763002, which
iterator over layout tree to encapsulate layout tree access from editing == no
layout/ reference from editing/ except for LayoutPosition.{h,cpp}.

[commit-bot: I haz the power, 2016-08-26 02:20:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 02:20:29]

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_...)

[joone, 2016-08-26 06:03:57]

On 2016/08/26 01:55:44, Yosi_UTC9 wrote:
> I'm not a fan of this patch because of introducing
> LayoutText::needToRestoreCollapsedSpace().
> I feel meaning of this API is vague.
> 

How about LayoutText::hasLineBreakforTextWrap?

[yosin_UTC9, 2016-08-26 09:39:20]

On 2016/08/26 at 06:03:57, joone.hur wrote:
> On 2016/08/26 01:55:44, Yosi_UTC9 wrote:
> > I'm not a fan of this patch because of introducing
> > LayoutText::needToRestoreCollapsedSpace().
> > I feel meaning of this API is vague.
> > 
> 
> How about LayoutText::hasLineBreakforTextWrap?

Since, LayoutText can spread into multiple lines,
this information can be store in RootInlineBox or last InlineTextBlock.

[joone, 2016-08-28 07:42:18]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 07:42:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-28 09:47:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-28 09:47:12]

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_...)

[joone, 2016-08-28 23:46:29]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 23:46:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-08-29 00:55:43]

On 2016/08/26 09:39:20, Yosi_UTC9 wrote:
> On 2016/08/26 at 06:03:57, joone.hur wrote:
> > On 2016/08/26 01:55:44, Yosi_UTC9 wrote:
> > > I'm not a fan of this patch because of introducing
> > > LayoutText::needToRestoreCollapsedSpace().
> > > I feel meaning of this API is vague.
> > > 
> > 
> > How about LayoutText::hasLineBreakforTextWrap?
> 
> Since, LayoutText can spread into multiple lines,
> this information can be store in RootInlineBox or last InlineTextBlock.

When BreakingContext::commitAndUpdateLineBreakIfNeeded() is called,
RootInlineBox seems not to be created yet so we are not able to set the line
break information in it.

[commit-bot: I haz the power, 2016-08-29 01:00:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 01:00:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yosin_UTC9, 2016-08-29 03:39:07]

yosin@chromium.org changed reviewers:
+ kojii@chromium.org

[yosin_UTC9, 2016-08-29 03:39:07]

+kojii@, as layout expert

[kojii, 2016-08-29 06:48:38]

I'm not very familiar with TextIterator, but two comments:

1. This patch sets a flag whenever a line wraps. I understand it helps clipboard
to handle easier, but the code is very hot, setting flags in case user may copy
the text looks too much. Can't the TextIterator figure it out without adding a
flag to LineLayoutText?

2. Can you add test cases for languages that do not use spaces, such as CJK, or
when author use word-break: break-all, that when line wraps without spaces, we
don't add spaces?

[joone, 2016-08-29 17:58:03]

On 2016/08/29 06:48:38, kojii wrote:
> I'm not very familiar with TextIterator, but two comments:
> 
> 1. This patch sets a flag whenever a line wraps. I understand it helps
clipboard
> to handle easier, but the code is very hot, setting flags in case user may
copy
> the text looks too much. Can't the TextIterator figure it out without adding a
> flag to LineLayoutText?
> 

I tried in https://codereview.chromium.org/2193033004/, but it can't deal with
other cases:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>

Also, there is a case that needs to restore the leading space:
<div style="width: 2em;"><b><i>^foo</i></b> bar|</div>

This one is more complicated because we need to check the leading and trailing
spaces:
<div style="width: 2em;"><b><i>foo </i></b> bar</div>

Without setting the flag, TextIterator can't handle those cases.


> 2. Can you add test cases for languages that do not use spaces, such as CJK,
or
> when author use word-break: break-all, that when line wraps without spaces, we
> don't add spaces?

Done.

[joone, 2016-08-29 17:58:29]

https://codereview.chromium.org/2280513004/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html
(right):

https://codereview.chromium.org/2280513004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:55:
'4. Space should not be added for CSS word-break: break-all');
kojii@ Here are the cases you wanted to test.

[joone, 2016-08-29 17:59:12]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 18:10:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-08-30 02:24:53]

On 2016/08/29 at 17:58:03, joone.hur wrote:
> On 2016/08/29 06:48:38, kojii wrote:
> > I'm not very familiar with TextIterator, but two comments:
> > 
> > 1. This patch sets a flag whenever a line wraps. I understand it helps
clipboard
> > to handle easier, but the code is very hot, setting flags in case user may
copy
> > the text looks too much. Can't the TextIterator figure it out without adding
a
> > flag to LineLayoutText?
> > 
> 
> I tried in https://codereview.chromium.org/2193033004/, but it can't deal with
other cases:
> <div style="width: 2em;"><b><i>foo </i></b>bar</div>
> 
> Also, there is a case that needs to restore the leading space:
> <div style="width: 2em;"><b><i>^foo</i></b> bar|</div>
> 
> This one is more complicated because we need to check the leading and trailing
spaces:
> <div style="width: 2em;"><b><i>foo </i></b> bar</div>
> 
> Without setting the flag, TextIterator can't handle those cases.

Instead of walking DOM tree, how about finding adjacent InlineBox, then get its
node?

[commit-bot: I haz the power, 2016-08-30 03:35:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:35:52]

Dry run: This issue passed the CQ dry run.

[joone, 2016-08-30 06:07:44]

On 2016/08/30 02:24:53, kojii wrote:
> On 2016/08/29 at 17:58:03, joone.hur wrote:
> > On 2016/08/29 06:48:38, kojii wrote:
> > > I'm not very familiar with TextIterator, but two comments:
> > > 
> > > 1. This patch sets a flag whenever a line wraps. I understand it helps
> clipboard
> > > to handle easier, but the code is very hot, setting flags in case user may
> copy
> > > the text looks too much. Can't the TextIterator figure it out without
adding
> a
> > > flag to LineLayoutText?
> > > 
> > 
> > I tried in https://codereview.chromium.org/2193033004/, but it can't deal
with
> other cases:
> > <div style="width: 2em;"><b><i>foo </i></b>bar</div>
> > 
> > Also, there is a case that needs to restore the leading space:
> > <div style="width: 2em;"><b><i>^foo</i></b> bar|</div>
> > 
> > This one is more complicated because we need to check the leading and
trailing
> spaces:
> > <div style="width: 2em;"><b><i>foo </i></b> bar</div>
> > 
> > Without setting the flag, TextIterator can't handle those cases.
> 
> Instead of walking DOM tree, how about finding adjacent InlineBox, then get
its
> node?

How do we know if the inlinebox has a line break for text wrap?

[kojii, 2016-08-30 06:18:53]

On 2016/08/30 at 06:07:44, joone.hur wrote:
> > 
> > Instead of walking DOM tree, how about finding adjacent InlineBox, then get
its
> > node?
> 
> How do we know if the inlinebox has a line break for text wrap?

I meant to replace |Strategy::nextSibling(*m_node)| with getting next InlineBox
and get node of it. At that point, the code is only for wrapped line, no?

[joone, 2016-08-30 06:53:18]

On 2016/08/30 06:18:53, kojii wrote:
> On 2016/08/30 at 06:07:44, joone.hur wrote:
> > > 
> > > Instead of walking DOM tree, how about finding adjacent InlineBox, then
get
> its
> > > node?
> > 
> > How do we know if the inlinebox has a line break for text wrap?
> 
> I meant to replace |Strategy::nextSibling(*m_node)| with getting next
InlineBox
> and get node of it. At that point, the code is only for wrapped line, no?

This code only handle the case that a line break is added between the text and
inline element.
In this case, we need to restore the tailing space when we copy it.
Here are examples:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter"></span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

This code has unintended effects like
https://codereview.chromium.org/2193033004/diff/200001/third_party/WebKit/Lay...
The inline element(span) doesn't have any text so we don't need to restore the
trailing space, but it is restored.
I think that we had better find the exact line break case during layout instead
of adding more conditions to TextIterator.

[kojii, 2016-08-30 07:13:36]

On 2016/08/30 at 06:53:18, joone.hur wrote:
> On 2016/08/30 06:18:53, kojii wrote:
> > On 2016/08/30 at 06:07:44, joone.hur wrote:
> > > > 
> > > > Instead of walking DOM tree, how about finding adjacent InlineBox, then
get
> > its
> > > > node?
> > > 
> > > How do we know if the inlinebox has a line break for text wrap?
> > 
> > I meant to replace |Strategy::nextSibling(*m_node)| with getting next
InlineBox
> > and get node of it. At that point, the code is only for wrapped line, no?
> 
> This code only handle the case that a line break is added between the text and
inline element.
> In this case, we need to restore the tailing space when we copy it.
> Here are examples:
> <p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
> <p>Counter: <span id="counter"></span></p>
> <dfn>world <kbd>Blink</kbd></dfn>
> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>
> 
> This code has unintended effects like
>
https://codereview.chromium.org/2193033004/diff/200001/third_party/WebKit/Lay...
> The inline element(span) doesn't have any text so we don't need to restore the
trailing space, but it is restored.
> I think that we had better find the exact line break case during layout
instead of adding more conditions to TextIterator.

I understand doing so looks the best approach for this code. However, it costs
CPU cycles on every document, so what I'm saying is even if it complicates this
code, it's better not to add cycles to line breaker.

Are you saying adding the flag in line breaker is better, or impossible without
doing so? If the latter, I don't understand why it's impossible.

[joone, 2016-08-31 03:18:57]

On 2016/08/30 07:13:36, kojii wrote:
> On 2016/08/30 at 06:53:18, joone.hur wrote:
> > On 2016/08/30 06:18:53, kojii wrote:
> > > On 2016/08/30 at 06:07:44, joone.hur wrote:
> > > > > 
> > > > > Instead of walking DOM tree, how about finding adjacent InlineBox,
then
> get
> > > its
> > > > > node?
> > > > 
> > > > How do we know if the inlinebox has a line break for text wrap?
> > > 
> > > I meant to replace |Strategy::nextSibling(*m_node)| with getting next
> InlineBox
> > > and get node of it. At that point, the code is only for wrapped line, no?
> > 
> > This code only handle the case that a line break is added between the text
and
> inline element.
> > In this case, we need to restore the tailing space when we copy it.
> > Here are examples:
> > <p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
> > <p>Counter: <span id="counter"></span></p>
> > <dfn>world <kbd>Blink</kbd></dfn>
> > <div>Copy this area <a
href="http://foo/">AVeryLongWordThatWillWrap</a></div>
> > 
> > This code has unintended effects like
> >
>
https://codereview.chromium.org/2193033004/diff/200001/third_party/WebKit/Lay...
> > The inline element(span) doesn't have any text so we don't need to restore
the
> trailing space, but it is restored.
> > I think that we had better find the exact line break case during layout
> instead of adding more conditions to TextIterator.
> 
> I understand doing so looks the best approach for this code. However, it costs
> CPU cycles on every document, so what I'm saying is even if it complicates
this
> code, it's better not to add cycles to line breaker.
> 
> Are you saying adding the flag in line breaker is better, or impossible
without
> doing so? If the latter, I don't understand why it's impossible.

I think it is better to add the flag in the line breaker because it exactly
knows when a line break is added for text wrap, but I didn't think about the
cost.
If you are okay with adding more conditions in TextIterator, it could handle
more cases.

[kojii, 2016-08-31 04:23:06]

On 2016/08/31 at 03:18:57, joone.hur wrote:
> > > I think that we had better find the exact line break case during layout
> > instead of adding more conditions to TextIterator.
> > 
> > I understand doing so looks the best approach for this code. However, it
costs
> > CPU cycles on every document, so what I'm saying is even if it complicates
this
> > code, it's better not to add cycles to line breaker.
> > 
> > Are you saying adding the flag in line breaker is better, or impossible
without
> > doing so? If the latter, I don't understand why it's impossible.
> 
> I think it is better to add the flag in the line breaker because it exactly
knows when a line break is added for text wrap, but I didn't think about the
cost.
> If you are okay with adding more conditions in TextIterator, it could handle
more cases.

Yeah, TextIterator runs only while editing, while LineBreaker runs everywhere.
Even if doing in TextIterator adds total lines of code, I think it helps better
performance overall.

Sorry for the troublesome.

[joone, 2016-09-01 09:07:26]

Patchset #6 (id:140001) has been deleted

[joone, 2016-09-01 09:10:22]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 09:10:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-09-01 09:13:24]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL sets a flag in LayoutText when a line break is added
instead of trailing space for a wrapped text. The flag can be
used to restore the trailing space when the text is copied(ctrl+c)
so it can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[commit-bot: I haz the power, 2016-09-01 09:32:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 09:32:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[joone, 2016-09-01 19:02:46]

Patchset #7 (id:180001) has been deleted

[joone, 2016-09-01 19:05:00]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 19:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-09-01 19:27:54]

On 2016/08/31 04:23:06, kojii wrote:
> 
> Yeah, TextIterator runs only while editing, while LineBreaker runs everywhere.
> Even if doing in TextIterator adds total lines of code, I think it helps
better
> performance overall.
> 
> Sorry for the troublesome.

Hi yosin@, kojii@

I've updated the CL in order not to change the layout code. It is better to use
InlineTextBox instead of walking DOM tree to find the LayoutText that has a line
break for text wrap: it doesn't need many conditions to handle the new case
below. Thanks!

<div style="width: 2em;"><b><i>foo </i></b>bar</div>

[commit-bot: I haz the power, 2016-09-01 21:18:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 21:18:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yosin_UTC9, 2016-09-02 04:14:11]

C++ code changes seems to be good.
Let's make test simpler.

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html
(right):

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:21:
var copy = selection.document.getElementById('copy');
Could you utilize |selection.setClipboardData()|?
See http://crrev.com/2303533003

[joone, 2016-09-02 07:32:50]

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html
(right):

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:21:
var copy = selection.document.getElementById('copy');
On 2016/09/02 04:14:10, Yosi_UTC9 wrote:
> Could you utilize |selection.setClipboardData()|?
> See http://crrev.com/2303533003

It doesn't work with this function because we only need to copy the tags under
<div style="width: 2em;">.

[yosin_UTC9, 2016-09-05 01:42:23]

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html
(right):

https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:21:
var copy = selection.document.getElementById('copy');
On 2016/09/02 at 07:32:50, joone wrote:
> On 2016/09/02 04:14:10, Yosi_UTC9 wrote:
> > Could you utilize |selection.setClipboardData()|?
> > See http://crrev.com/2303533003
> 
> It doesn't work with this function because we only need to copy the tags under
<div style="width: 2em;">.

assert_selection(
  '<div style="width: 2em;"><b><i>foo </i></b>bar</div><div>|</div>',
  selection => {
     selection.setClipboardData('<b><i>foo</i></b>bar');
     selection.document.execCommand('paste');
  },
  ...);

Should work

[joone, 2016-09-06 07:59:52]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 08:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-09-06 08:05:02]

On 2016/09/05 01:42:23, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html
> (right):
> 
>
https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:21:
> var copy = selection.document.getElementById('copy');
> On 2016/09/02 at 07:32:50, joone wrote:
> > On 2016/09/02 04:14:10, Yosi_UTC9 wrote:
> > > Could you utilize |selection.setClipboardData()|?
> > > See http://crrev.com/2303533003
> > 
> > It doesn't work with this function because we only need to copy the tags
under
> <div style="width: 2em;">.
> 
> assert_selection(
>   '<div style="width: 2em;"><b><i>foo </i></b>bar</div><div>|</div>',
>   selection => {
>      selection.setClipboardData('<b><i>foo</i></b>bar');
>      selection.document.execCommand('paste');
>   },
>   ...);
> 
> Should work

selection.setClipboardData works fine in the updated test case. Please take a
look at it

[yosin_UTC9, 2016-09-06 09:15:21]

lgtm

Thanks!

[commit-bot: I haz the power, 2016-09-06 09:38:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 09:38:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-09-06 21:27:10]

The CQ bit was checked by joone.hur@intel.com

[commit-bot: I haz the power, 2016-09-06 21:27:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:04:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 23:04:49]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-09-07 00:13:00]

On 2016/09/06 23:04:49, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

yosin@ there is a test fail in try jobs on Mac as follows:
unexpected_failures:
accessibility/element-role-mapping-focusable.html

However, I don't see any test fails on my Mac.
Can I add this test fail to TestExpectations file?

[joone, 2016-09-07 01:05:39]

On 2016/09/07 00:13:00, joone wrote:
> On 2016/09/06 23:04:49, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> yosin@ there is a test fail in try jobs on Mac as follows:
> unexpected_failures:
> accessibility/element-role-mapping-focusable.html
> 
> However, I don't see any test fails on my Mac.
> Can I add this test fail to TestExpectations file?

It looks like the dump tree result( is a bit different:
element-role-mapping-focusable-expected.txt

Regressions: Unexpected text-only failures (1)
  accessibility/element-role-mapping-focusable.html [ Failure ]

We could avoid this problem by rebaselining this test.

[joone, 2016-09-07 01:12:05]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 01:12:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 01:15:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 01:15:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-09-07 01:18:31]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 01:18:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 02:49:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 02:49:30]

Dry run: This issue passed the CQ dry run.

[joone, 2016-09-07 08:47:52]

The CQ bit was checked by joone.hur@intel.com

[joone, 2016-09-07 08:47:52]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2280513004/#ps260001
(title: "Rebaseline for Mac")

[commit-bot: I haz the power, 2016-09-07 08:48:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 08:51:56]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

[commit-bot: I haz the power, 2016-09-07 08:51:57]

Committed patchset #10 (id:260001)

[commit-bot: I haz the power, 2016-09-07 08:53:32]

Description was changed from

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
==========

to

==========
Restore a collapsed trailing space of text used for line break

When a text is wrapped during layout, the trailing space of the
text can be collapsed and a line break is inserted instead of
the space. In this case, we need to restore the collapsed space
when we copy the text. This problem was fixed in the below CL:
https://codereview.chromium.org/2193033004/

However, the CL only deals with the case that the sibling of
the text is an inline element:
<p>My favorite browser is <del>ABC</del> <ins>Chrome</ins>!</p>
<p>Counter: <span id="counter">10</span></p>
<dfn>world <kbd>Blink</kbd></dfn>
<div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div>

In addition, the previous fix has the unintended effect that the
trailing space can be restored even if there is no line break.

This CL can additionally handle the following case:
<div style="width: 2em;"><b><i>foo </i></b>bar</div>
It also removes the unintended effect.

BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html

Committed: https://crrev.com/9b93a675c78452c27f1749ff243ab8f9449b1184
Cr-Commit-Position: refs/heads/master@{#416883}
==========

[commit-bot: I haz the power, 2016-09-07 08:53:34]

Patchset 10 (id:??) landed as
https://crrev.com/9b93a675c78452c27f1749ff243ab8f9449b1184
Cr-Commit-Position: refs/heads/master@{#416883}