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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 months ago by joone
Modified:
11 months, 2 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 ...
12 months 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 ...
12 months 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 ...
12 months 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 ...
12 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 ...
11 months, 4 weeks ago (2016-08-29 00:55:43 UTC) #25
yosin_UTC9
+kojii@, as layout expert
11 months, 4 weeks 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 ...
11 months, 4 weeks 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: ...
11 months, 4 weeks 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: ...
11 months, 4 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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, ...
11 months, 3 weeks 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: > > > ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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): ...
11 months, 3 weeks 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: ...
11 months, 3 weeks 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 ...
11 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): > > ...
11 months, 2 weeks ago (2016-09-06 08:05:02 UTC) #61
yosin_UTC9
lgtm Thanks!
11 months, 2 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
11 months, 2 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)
11 months, 2 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 ...
11 months, 2 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: ...
11 months, 2 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
11 months, 2 weeks ago (2016-09-07 08:48:04 UTC) #81
commit-bot: I haz the power
Committed patchset #10 (id:260001)
11 months, 2 weeks ago (2016-09-07 08:51:57 UTC) #83
commit-bot: I haz the power
11 months, 2 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 b40b6558b