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

Issue 2280513004: Restore a collapsed trailing space of text used for line break (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 1 week ago by joone
Modified:
9 months, 3 weeks ago
Reviewers:
yosin_UTC9, esprehn, kojii
CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Update comment #

Patch Set 3 : Introduce hasLineBreakForTextWrap #

Patch Set 4 : Fix test fails #

Patch Set 5 : Add CJK and word-break: break-all cases #

Total comments: 1

Patch Set 6 : Don't touch layout code #

Patch Set 7 : fix test fail #

Total comments: 3

Patch Set 8 : Use selection.setClipboardData() #

Patch Set 9 : Rebaseline for Mac #

Patch Set 10 : Rebaseline for Mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -19 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/compositing/geometry/assert-marquee-timer-expected.txt View 1 2 3 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
M third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html View 1 2 3 4 5 6 7 1 chunk +30 lines, -6 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 3 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 3 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 3 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 1 chunk +3 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 85 (56 generated)
joone
Hi, This is a better way of fixing http://crbug.com/318925, which handles more cases. Please take ...
10 months, 1 week ago (2016-08-26 00:41:17 UTC) #9
yosin_UTC9
I'm not a fan of this patch because of introducing LayoutText::needToRestoreCollapsedSpace(). I feel meaning of ...
10 months, 1 week ago (2016-08-26 01:55:44 UTC) #14
joone
On 2016/08/26 01:55:44, Yosi_UTC9 wrote: > I'm not a fan of this patch because of ...
10 months, 1 week ago (2016-08-26 06:03:57 UTC) #17
yosin_UTC9
On 2016/08/26 at 06:03:57, joone.hur wrote: > On 2016/08/26 01:55:44, Yosi_UTC9 wrote: > > I'm ...
10 months ago (2016-08-26 09:39:20 UTC) #18
joone
On 2016/08/26 09:39:20, Yosi_UTC9 wrote: > On 2016/08/26 at 06:03:57, joone.hur wrote: > > On ...
10 months ago (2016-08-29 00:55:43 UTC) #25
yosin_UTC9
+kojii@, as layout expert
10 months ago (2016-08-29 03:39:07 UTC) #29
kojii
I'm not very familiar with TextIterator, but two comments: 1. This patch sets a flag ...
10 months ago (2016-08-29 06:48:38 UTC) #30
joone
On 2016/08/29 06:48:38, kojii wrote: > I'm not very familiar with TextIterator, but two comments: ...
10 months ago (2016-08-29 17:58:03 UTC) #31
joone
https://codereview.chromium.org/2280513004/diff/120001/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/2280513004/diff/120001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html#newcode55 third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html:55: '4. Space should not be added for CSS word-break: ...
10 months ago (2016-08-29 17:58:29 UTC) #32
kojii
On 2016/08/29 at 17:58:03, joone.hur wrote: > On 2016/08/29 06:48:38, kojii wrote: > > I'm ...
10 months ago (2016-08-30 02:24:53 UTC) #35
joone
On 2016/08/30 02:24:53, kojii wrote: > On 2016/08/29 at 17:58:03, joone.hur wrote: > > On ...
10 months ago (2016-08-30 06:07:44 UTC) #38
kojii
On 2016/08/30 at 06:07:44, joone.hur wrote: > > > > Instead of walking DOM tree, ...
10 months ago (2016-08-30 06:18:53 UTC) #39
joone
On 2016/08/30 06:18:53, kojii wrote: > On 2016/08/30 at 06:07:44, joone.hur wrote: > > > ...
10 months ago (2016-08-30 06:53:18 UTC) #40
kojii
On 2016/08/30 at 06:53:18, joone.hur wrote: > On 2016/08/30 06:18:53, kojii wrote: > > On ...
10 months ago (2016-08-30 07:13:36 UTC) #41
joone
On 2016/08/30 07:13:36, kojii wrote: > On 2016/08/30 at 06:53:18, joone.hur wrote: > > On ...
10 months ago (2016-08-31 03:18:57 UTC) #42
kojii
On 2016/08/31 at 03:18:57, joone.hur wrote: > > > I think that we had better ...
10 months ago (2016-08-31 04:23:06 UTC) #43
joone
On 2016/08/31 04:23:06, kojii wrote: > > Yeah, TextIterator runs only while editing, while LineBreaker ...
10 months ago (2016-09-01 19:27:54 UTC) #53
yosin_UTC9
C++ code changes seems to be good. Let's make test simpler. https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html File third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html (right): ...
10 months ago (2016-09-02 04:14:11 UTC) #56
joone
https://codereview.chromium.org/2280513004/diff/200001/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/2280513004/diff/200001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html#newcode21 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: ...
10 months ago (2016-09-02 07:32:50 UTC) #57
yosin_UTC9
https://codereview.chromium.org/2280513004/diff/200001/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/2280513004/diff/200001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html#newcode21 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 ...
9 months, 3 weeks ago (2016-09-05 01:42:23 UTC) #58
joone
On 2016/09/05 01:42:23, Yosi_UTC9 wrote: > https://codereview.chromium.org/2280513004/diff/200001/third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html > File > third_party/WebKit/LayoutTests/editing/pasteboard/preserve-trailing-space.html > (right): > > ...
9 months, 3 weeks ago (2016-09-06 08:05:02 UTC) #61
yosin_UTC9
lgtm Thanks!
9 months, 3 weeks ago (2016-09-06 09:15:21 UTC) #62
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/2280513004/220001
9 months, 3 weeks ago (2016-09-06 21:27:34 UTC) #66
commit-bot: I haz the power
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_ng/builds/291479)
9 months, 3 weeks ago (2016-09-06 23:04:49 UTC) #68
joone
On 2016/09/06 23:04:49, commit-bot: I haz the power wrote: > Try jobs failed on following ...
9 months, 3 weeks ago (2016-09-07 00:13:00 UTC) #69
joone
On 2016/09/07 00:13:00, joone wrote: > On 2016/09/06 23:04:49, commit-bot: I haz the power wrote: ...
9 months, 3 weeks ago (2016-09-07 01:05:39 UTC) #70
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/2280513004/260001
9 months, 3 weeks ago (2016-09-07 08:48:04 UTC) #81
commit-bot: I haz the power
Committed patchset #10 (id:260001)
9 months, 3 weeks ago (2016-09-07 08:51:57 UTC) #83
commit-bot: I haz the power
9 months, 3 weeks ago (2016-09-07 08:53:34 UTC) #85
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/9b93a675c78452c27f1749ff243ab8f9449b1184
Cr-Commit-Position: refs/heads/master@{#416883}
Sign in to reply to this message.

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