Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2193033004: Restore the collapsed spaces of the text when we copy it (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months ago by joone
Modified:
10 months, 1 week ago
CC:
blink-reviews, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -15 lines) Patch
M content/renderer/render_view_browsertest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M content/test/data/accessibility/html/frameset-expected-blink.txt View 1 1 chunk +2 lines, -2 lines 0 comments Download
M content/test/data/accessibility/html/frameset-expected-mac.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/test/data/accessibility/html/frameset-expected-win.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/compositing/geometry/assert-marquee-timer-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/editing/execCommand/remove-format-multiple-elements-mac-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/editing/execCommand/script-tests/remove-format-multiple-elements-mac.js View 1 chunk +2 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html View 1 2 3 4 5 6 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/editing/selection/caret-at-bidi-boundary-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/shadow/pseudo-not-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/js/inc-bracket-assign-subscript-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/js/post-inc-assign-overwrites-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -1 line 1 comment Download
Trybot results:  linux_chromium_rel_ng   win_chromium_x64_rel_ng   mac_chromium_compile_dbg_ng   ios-device   ios-simulator   mac_chromium_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   linux_chromium_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_clobber_rel_ng   linux_chromium_chromeos_rel_ng   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   linux_android_rel_ng   cast_shell_android   android_clang_dbg_recipe   android_arm64_dbg_recipe   android_compile_dbg   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-device   ios-simulator   mac_chromium_rel_ng   linux_android_rel_ng   android_clang_dbg_recipe   cast_shell_android   android_arm64_dbg_recipe   android_compile_dbg   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   linux_chromium_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_clobber_rel_ng   linux_chromium_chromeos_rel_ng 
Commit queue not available (can’t edit this change).

Messages

Total messages: 83 (57 generated)
joone
Hi, yosin@ could you take a look at this CL?
11 months ago (2016-08-02 19:17:30 UTC) #25
joone
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp#newcode616 third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(m_node->nextSibling())) { This problem happens if ...
11 months ago (2016-08-02 20:31:42 UTC) #26
yosin_UTC9
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html#newcode12 third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:12: var range = document.createRange(); Since you don't use |range| ...
11 months ago (2016-08-03 01:16:23 UTC) #27
joone
Thanks for review. Could you take a look at the change? https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): ...
10 months, 4 weeks ago (2016-08-03 21:51:00 UTC) #28
joone
https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/60001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp#newcode616 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 ...
10 months, 4 weeks ago (2016-08-03 22:37:41 UTC) #33
joone
On 2016/08/03 22:37:41, joone wrote: > > When we reduce the width, the same problem ...
10 months, 4 weeks ago (2016-08-04 00:21:51 UTC) #36
yosin_UTC9
On 2016/08/04 at 00:21:51, joone.hur wrote: > On 2016/08/03 22:37:41, joone wrote: > > > ...
10 months, 4 weeks ago (2016-08-04 01:18:35 UTC) #37
yosin_UTC9
lgtm https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): https://codereview.chromium.org/2193033004/diff/100001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html#newcode9 third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:9: var copy = selection.document.getElementById('copy'); nit: Please remove this ...
10 months, 4 weeks ago (2016-08-04 01:18:45 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2193033004/120001
10 months, 4 weeks ago (2016-08-04 05:30:29 UTC) #45
commit-bot: I haz the power
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_presubmit/builds/230857)
10 months, 4 weeks ago (2016-08-04 05:35:55 UTC) #47
joone
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/LayoutTests/editing/pasteboard/preserve-trailing-space.html File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html ...
10 months, 4 weeks ago (2016-08-04 06:18:22 UTC) #49
joone
https://codereview.chromium.org/2193033004/diff/120001/content/renderer/render_view_browsertest.cc File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/2193033004/diff/120001/content/renderer/render_view_browsertest.cc#newcode1735 content/renderer/render_view_browsertest.cc:1735: EXPECT_EQ(output, "hello \n\nworld"); The space is added when iframe ...
10 months, 4 weeks ago (2016-08-04 16:43:05 UTC) #50
esprehn
https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp#newcode616 third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:616: if (m_node->nextSibling() && isInline(Strategy::next(*m_node))) { What is this nextSibling ...
10 months, 4 weeks ago (2016-08-04 20:54:27 UTC) #51
joone
On 2016/08/04 20:54:27, esprehn wrote: > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp > File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right): > > https://codereview.chromium.org/2193033004/diff/120001/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp#newcode616 > ...
10 months, 4 weeks ago (2016-08-04 21:57:28 UTC) #52
esprehn
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/Source/core/editing/iterators/TextIterator.cpp ...
10 months, 4 weeks ago (2016-08-04 21:59:01 UTC) #53
joone
On 2016/08/04 21:59:01, esprehn wrote: > On 2016/08/04 at 21:57:28, joone.hur wrote: > > On ...
10 months, 4 weeks ago (2016-08-04 22:23:45 UTC) #55
esprehn
lgtm w/ the endsWith change, aboxhall@ should verify that the change you're making to AX ...
10 months, 4 weeks ago (2016-08-04 22:35:27 UTC) #57
joone
Hi all, the CL was updated a bit. Please take a look at the change. ...
10 months, 3 weeks ago (2016-08-05 18:37:48 UTC) #62
joone
Hi aboxhall@, could you review this CL?
10 months, 3 weeks ago (2016-08-10 17:17:01 UTC) #71
joone
On 2016/08/10 17:17:01, joone wrote: > Hi aboxhall@, could you review this CL? esprehn@ could ...
10 months, 2 weeks ago (2016-08-16 19:02:00 UTC) #72
joone
Hi dmazzoni@ could you review this CL?
10 months, 1 week ago (2016-08-20 00:29:40 UTC) #74
dmazzoni
lgtm Accessibility changes lgtm, this is an improvement!
10 months, 1 week ago (2016-08-22 19:25:13 UTC) #75
joone
On 2016/08/22 19:25:13, dmazzoni wrote: > lgtm > > Accessibility changes lgtm, this is an ...
10 months, 1 week ago (2016-08-22 19:40:07 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2193033004/200001
10 months, 1 week ago (2016-08-22 19:40:08 UTC) #79
commit-bot: I haz the power
Committed patchset #10 (id:200001)
10 months, 1 week ago (2016-08-22 23:15:03 UTC) #81
commit-bot: I haz the power
10 months, 1 week ago (2016-08-22 23:17:25 UTC) #83
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/6c91ec8582845dcaa0cc8a4b9232cd844f6bf57c
Cr-Commit-Position: refs/heads/master@{#413572}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589