Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2193033004:
Restore the collapsed spaces of the text when we copy it (Closed)
DescriptionRestore the collapsed spaces of the text when we copy it
When we copy a wrapped text(text + inline element), the trailing
spaces of the text are collapsed during layout. However, when
we copy the text, there is no space between the text and the
inline element such as <a>, <del>, <span>, and etc so we need
to restore the trailing space of the text.
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>
BUG=318925
TEST=editing/pasteboard/preserve-trailing-space.html
Committed: https://crrev.com/6c91ec8582845dcaa0cc8a4b9232cd844f6bf57c
Cr-Commit-Position: refs/heads/master@{#413572}
Patch Set 1 #Patch Set 2 : new baseline #Patch Set 3 : Fix test fails in Linux #Patch Set 4 : Fix test fail in Mac and Win #
Total comments: 8
Patch Set 5 : Add test cases for <div><b><i>foo </i></b> bar</div> #
Total comments: 1
Patch Set 6 : Remove the test cases #
Total comments: 2
Patch Set 7 : remove the copy variable in the test case #
Total comments: 2
Patch Set 8 : The string check first #
Total comments: 2
Patch Set 9 : Use of str.endsWith() #Patch Set 10 : Fix test fails #
Total comments: 1
Messages
Total messages: 83 (57 generated)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't trim the plain space at the end of string When we copy and paste selected tags, we need to preserve the plain space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Don't trim the plain space at the end of string When we copy and paste selected tags, we need to preserve the plain space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Don't trim the plain space at the end of string When we copy and paste selected tags, we need to preserve the plain space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Don't trim the space at the end of string When we copy and paste selected tags, we need to preserve the plain space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Don't trim the space at the end of string When we copy and paste selected tags, we need to preserve the plain space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Don't trim the space at the end of string When we copy and paste selected tags, we need to preserve the space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
joone.hur@intel.com changed reviewers: + yosin@chromium.org
Hi, yosin@ could you take a look at this CL?
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(m_node->nextSibling())) { This problem happens if the text node's next sibling is inline element like <a>, <del>, <span> and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> So, if subrunEnd points the last character of the text node and the last character is a plain space and there are not double spaces (it fixes some test fails), we should include the last space by adding 1 to subrunEnd.
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:12: var range = document.createRange(); Since you don't use |range| other than setting |selection|, you can specify selection by using "^" and "|" in sample input. https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(m_node->nextSibling())) { On 2016/08/02 at 20:31:42, joone wrote: > This problem happens if the text node's next sibling is inline element like <a>, <del>, <span> and etc > > 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>WebKit</kbd></dfn> > <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> > > So, if subrunEnd points the last character of the text node and the last character is a plain space and there are not double spaces (it fixes some test fails), we should include the last space by adding 1 to subrunEnd. How about this case? <div><b><i>foo </i></b> bar</div> Should we check following node of |m_node|, .e.g isInline(Strategy::next(*m_node))? https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:617: if (str[str.length() - 1] == ' ' && subrunEnd == str.length() - 1 && str[subrunEnd-1] != ' ') nit: s/subrunEnd-1/subrunEnd - 1/
Thanks for review. Could you take a look at the change? https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:12: var range = document.createRange(); On 2016/08/03 01:16:23, Yosi_UTC9 wrote: > Since you don't use |range| other than setting |selection|, you can specify > selection by using "^" and "|" in sample input. Done. https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(m_node->nextSibling())) { On 2016/08/03 01:16:23, Yosi_UTC9 wrote: > On 2016/08/02 at 20:31:42, joone wrote: > > This problem happens if the text node's next sibling is inline element like > <a>, <del>, <span> and etc > > > > 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>WebKit</kbd></dfn> > > <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> > > > > So, if subrunEnd points the last character of the text node and the last > character is a plain space and there are not double spaces (it fixes some test > fails), we should include the last space by adding 1 to subrunEnd. > > How about this case? > <div><b><i>foo </i></b> bar</div> Works fine. I added this case to the test file. > Should we check following node of |m_node|, .e.g > isInline(Strategy::next(*m_node))? > Done https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:617: if (str[str.length() - 1] == ' ' && subrunEnd == str.length() - 1 && str[subrunEnd-1] != ' ') On 2016/08/03 01:16:23, Yosi_UTC9 wrote: > nit: s/subrunEnd-1/subrunEnd - 1/ Done. https://codereview.chromium.org/2193033004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:56: '<div><b><i>foo </i></b> bar</div><div contenteditable id="editor"><b><i>foo\u00A0</i></b>bar|</div>'), In this case, only one space is reserved.
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't trim the space at the end of string When we copy and paste selected tags, we need to preserve the space at the end of string. BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during rendering. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Restore the collapsed spaces of the text we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during rendering. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during rendering. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(m_node->nextSibling())) { On 2016/08/03 21:50:59, joone wrote: > On 2016/08/03 01:16:23, Yosi_UTC9 wrote: > > On 2016/08/02 at 20:31:42, joone wrote: > > > This problem happens if the text node's next sibling is inline element like > > <a>, <del>, <span> and etc > > > > > > 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>WebKit</kbd></dfn> > > > <div>Copy this area <a > href="http://foo/">AVeryLongWordThatWillWrap</a></div> > > > > > > So, if subrunEnd points the last character of the text node and the last > > character is a plain space and there are not double spaces (it fixes some test > > fails), we should include the last space by adding 1 to subrunEnd. > > > > How about this case? > > <div><b><i>foo </i></b> bar</div> > > Works fine. I added this case to the test file. > > > Should we check following node of |m_node|, .e.g > > isInline(Strategy::next(*m_node))? > > > > Done When we reduce the width, the same problem happens: expected <div style="width: 2em;"><b><i>foo </i></b> bar</div><div contenteditable id="editor"><b><i>foo </i></b>bar|</div>, but got <div style="width: 2em;"><b><i>foo </i></b> bar</div><div contenteditable id="editor"><b><i>foo</i></b>bar|</div>,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/03 22:37:41, joone wrote: > > When we reduce the width, the same problem happens: > expected <div style="width: 2em;"><b><i>foo </i></b> bar</div><div > contenteditable id="editor"><b><i>foo </i></b>bar|</div>, > but got <div style="width: 2em;"><b><i>foo </i></b> bar</div><div > contenteditable id="editor"><b><i>foo</i></b>bar|</div>, We could solve the above case in a separate CL.
On 2016/08/04 at 00:21:51, joone.hur wrote: > On 2016/08/03 22:37:41, joone wrote: > > > > When we reduce the width, the same problem happens: > > expected <div style="width: 2em;"><b><i>foo </i></b> bar</div><div > > contenteditable id="editor"><b><i>foo </i></b>bar|</div>, > > but got <div style="width: 2em;"><b><i>foo </i></b> bar</div><div > > contenteditable id="editor"><b><i>foo</i></b>bar|</div>, > > We could solve the above case in a separate CL. Could you file a bug of this for tracking? Thanks.
lgtm https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:9: var copy = selection.document.getElementById('copy'); nit: Please remove this statement since variable |copy| isn't used.
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joone.hur@intel.com
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/2193033004/#ps120001 (title: "remove the copy variable in the test case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
joone.hur@intel.com changed reviewers: + esprehn@chromium.org
Hi esprehn@ could you take a look at the change in content/renderer/render_view_browsertest.cc? https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:9: var copy = selection.document.getElementById('copy'); On 2016/08/04 01:18:45, Yosi_UTC9 wrote: > nit: Please remove this statement since variable |copy| isn't used. Done.
https://codereview.chromium.org/2193033004/diff/120001/content/renderer/rende... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/2193033004/diff/120001/content/renderer/rende... content/renderer/render_view_browsertest.cc:1735: EXPECT_EQ(output, "hello \n\nworld"); The space is added when iframe is rendered. I thought it is a bug so I tried to fix it, but it isn't: https://codereview.chromium.org/2207513002/
https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(Strategy::next(*m_node))) { What is this nextSibling check about? The next() thing with the Strategy is not necessarily nextSibling, for example it might be display none, or a comment, or ShadowDOM could be projecting something here.
On 2016/08/04 20:54:27, esprehn wrote: > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if > (m_node->nextSibling() && isInline(Strategy::next(*m_node))) { > What is this nextSibling check about? The next() thing with the Strategy is not > necessarily nextSibling, for example it might be display none, or a comment, or > ShadowDOM could be projecting something here. It should have been "(Strategy::next(*m_node) && isInline(Strategy::next(*m_node)))" I'll update the CL soon.
On 2016/08/04 at 21:57:28, joone.hur wrote: > On 2016/08/04 20:54:27, esprehn wrote: > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): > > > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if > > (m_node->nextSibling() && isInline(Strategy::next(*m_node))) { > > What is this nextSibling check about? The next() thing with the Strategy is not > > necessarily nextSibling, for example it might be display none, or a comment, or > > ShadowDOM could be projecting something here. > > It should have been "(Strategy::next(*m_node) && isInline(Strategy::next(*m_node)))" > I'll update the CL soon. Calling it can be expensive btw, you'll want to save it to a local variable. The string check inside it is also much cheaper, I think you want to invert the logic and check that before calling next()
Patchset #8 (id:140001) has been deleted
On 2016/08/04 21:59:01, esprehn wrote: > On 2016/08/04 at 21:57:28, joone.hur wrote: > > On 2016/08/04 20:54:27, esprehn wrote: > > > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp > (right): > > > > > > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if > > > (m_node->nextSibling() && isInline(Strategy::next(*m_node))) { > > > What is this nextSibling check about? The next() thing with the Strategy is > not > > > necessarily nextSibling, for example it might be display none, or a comment, > or > > > ShadowDOM could be projecting something here. > > > > It should have been "(Strategy::next(*m_node) && > isInline(Strategy::next(*m_node)))" > > I'll update the CL soon. > > Calling it can be expensive btw, you'll want to save it to a local variable. The > string check inside it is also much cheaper, I think you want to invert the > logic and check that before calling next() esprehn@ Please take a look at the updated CL.
esprehn@chromium.org changed reviewers: + aboxhall@chromium.org
lgtm w/ the endsWith change, aboxhall@ should verify that the change you're making to AX makes sense though, now we're keeping around whitespace there in the AX tree? https://codereview.chromium.org/2193033004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (str[str.length() - 1] == ' ' && subrunEnd == str.length() - 1 && str[subrunEnd - 1] != ' ') { str.endsWith(' ') && ...
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Hi all, the CL was updated a bit. Please take a look at the change. Thanks! https://codereview.chromium.org/2193033004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (str[str.length() - 1] == ' ' && subrunEnd == str.length() - 1 && str[subrunEnd - 1] != ' ') { On 2016/08/04 22:35:27, esprehn wrote: > str.endsWith(' ') && ... Done. https://codereview.chromium.org/2193033004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:617: Node* nextNode = Strategy::nextSibling(*m_node); We should call nextSibling here, which can fix the test fails.
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during rendering. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, we need to restore the collapsed spaces in case that the text node's next sibling is inline element such as <a>, <del>, <span>, and etc 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the tailing space of the text. 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the tailing space of the text. 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>WebKit</kbd></dfn> <div>Copy this area <a href="http://foo/">AVeryLongWordThatWillWrap</a></div> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the tailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the tailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the tailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the trailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the trailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Hi aboxhall@, could you review this CL?
On 2016/08/10 17:17:01, joone wrote: > Hi aboxhall@, could you review this CL? esprehn@ could you ask aboxhall@ or other reviewers to review my CL?
joone.hur@intel.com changed reviewers: + dmazzoni@chromium.org
Hi dmazzoni@ could you review this CL?
lgtm Accessibility changes lgtm, this is an improvement!
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2193033004/#ps200001 (title: "Fix test fails")
On 2016/08/22 19:25:13, dmazzoni wrote: > lgtm > > Accessibility changes lgtm, this is an improvement! Thanks for review!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the trailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the trailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the trailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the trailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the trailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the trailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html ========== to ========== Restore the collapsed spaces of the text when we copy it When we copy a wrapped text(text + inline element), the trailing spaces of the text are collapsed during layout. However, when we copy the text, there is no space between the text and the inline element such as <a>, <del>, <span>, and etc so we need to restore the trailing space of the text. 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> BUG=318925 TEST=editing/pasteboard/preserve-trailing-space.html Committed: https://crrev.com/6c91ec8582845dcaa0cc8a4b9232cd844f6bf57c Cr-Commit-Position: refs/heads/master@{#413572} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6c91ec8582845dcaa0cc8a4b9232cd844f6bf57c Cr-Commit-Position: refs/heads/master@{#413572} |
