New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment
link

Issue 913995: [LayoutNG] Issue with pre-wrap inside NGLineBreaker(?) and floats.

Reported by ikilpatrick@chromium.org, Dec 11 Project Member

Issue description

See reduced test case.

I saw this on:
https://rome.ro/news/2018/12/10/reflections-on-dooms-development#

I narrowed it down to the behaviour with pre-wrap inside of the NGLineBreaker (I think),
only quickly looked inside the debugger.

It seems like when reshaping the line, or adding the end space, it somehow determines it can't fit anymore, and fallsback to taking up the max available width.

If you can't repo w/ the test case, resize window to trigger.
 
pre-wrap-tc.html
484 bytes View Download

Comment 1 by kojii@chromium.org, Dec 12

Cc: -kojii@chromium.org
Owner: kojii@chromium.org
Status: Assigned (was: Untriaged)
Thank you for finding this, I'll take a look.

Comment 2 by kojii@chromium.org, Dec 12

It's interesting to know that some pages use pre-wrap for the whole text. I wasn't aware such pages exist.

When trailing spaces are preserved, NGLineInfo::Width() includes it, and float code checks it with the available width. In this case, it's not an overflow from NGLineBreaker perspective, but float thinks the line overflows and push it down. This part is easy to fix.

This Width() is also used for 'text-align', and fun starts. IIRC it should exclude preserved spaces so that visible text is center/right-aligned. We have tests for it and I coded to make the tests pass...but as I test now, only WebKit does it. Legacy, Edge, Gecko all includes preserved spaces to compute center/right. Spec doesn't say anything. Were I...dreaming?? And we have no tests for this?? I'll check further.

Comment 3 by kojii@chromium.org, Dec 12

Cc: mstensho@chromium.org cbiesin...@chromium.org
NG            Blink          Edge           Gecko          WebKit
float       Include       Do not include Do not include Do not include Do not include
text-align  If !auto-wrap Include        Include        Include        Do not include
min-content Include       If !auto-wrap  If !auto-wrap  Include        If !auto-wrap
max-content Include       Include        Include        Include        Include

float is easy, NG should fix. max-content is interoperable.

text-align and min-content...match to legacy, add tests if none, and file an issue to csswg? Adding experts for opinions.

Comment 4 by e...@chromium.org, Dec 12

LGTM

Comment 5 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00f271c0f21dae086f84b60afa63e325fb06b730

commit 00f271c0f21dae086f84b60afa63e325fb06b730
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Dec 13 01:51:51 2018

[LayoutNG] Exclude trailing preserved spaces when avoiding floats

This patch fixes an interoperability issue where trailing
preserved spaces should be excluded when avoiding floats.
All 4 implementations are interoperable on this regard.

This patch adds NGLineInfo::HasOverflow(), because knowing
whether the line has overflow or not is easy and fast,
while knowing the exact inline size excluding trailing
preserved spaces may involve reshaping.

Other cases also have interoperability problems, such as
`text-align` or `min-content`. I'll investigate them in
following patches.

Bug:  913995 
Change-Id: Ibf13f1fcc28bbd99d44b56611141efa49d6d1c89
Reviewed-on: https://chromium-review.googlesource.com/c/1374329
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616155}
[modify] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.h
[modify] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
[modify] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc
[add] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/web_tests/external/wpt/css/css-text/white-space/pre-float-001.html
[add] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/web_tests/external/wpt/css/css-text/white-space/pre-wrap-float-001.html
[add] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/web_tests/external/wpt/css/css-text/white-space/reference/pre-float-001-ref.html
[add] https://crrev.com/00f271c0f21dae086f84b60afa63e325fb06b730/third_party/blink/web_tests/external/wpt/css/css-text/white-space/reference/pre-wrap-float-001-ref.html

Comment 7 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/766f3c614c1e21111fdff1af71e61f12720daf85

commit 766f3c614c1e21111fdff1af71e61f12720daf85
Author: Koji Ishii <kojii@chromium.org>
Date: Fri Dec 14 09:12:22 2018

[LayoutNG] Fix 'text-align' applied to 'white-space: pre-wrap'

This patch matches the behavior of 'text-align' applied to
'white-space: pre-wrap' and when the line wraps to the
current engine; do not include for 'justify' but do include
for other values.

Spec is not clear on this regard. An issue filed at:
https://github.com/w3c/csswg-drafts/issues/3440

It turned out that our tests cover the combination of the
properties only when lines do not wrap.

Because NGLineBreaker computes |has_only_trailing_spaces| as
part of the line breaking, the last lines and single lines
don't have such item. The difference caused the bug, but the
lack of tests prevented finding the problem. This patch adds
tests for this case.

Also, computing trailing space is moved to NGLineInfo as we
discovered it is needed in other cases too, with more cases
covered and with unit tests.

Bug:  913995 
Change-Id: I49428f2dcf193e2b7a745431f82724308a17d90f
Reviewed-on: https://chromium-review.googlesource.com/c/1374331
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616617}
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.cc
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.h
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.h
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker_test.cc
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/ng_length_utils.cc
[modify] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/renderer/core/layout/ng/ng_length_utils.h
[add] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/web_tests/fast/css/text-align-pre-trailing-spaces-expected.html
[add] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/web_tests/fast/css/text-align-pre-trailing-spaces.html
[add] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/web_tests/fast/css/text-align-pre-wrap-trailing-spaces-multi-lines-expected.html
[add] https://crrev.com/766f3c614c1e21111fdff1af71e61f12720daf85/third_party/blink/web_tests/fast/css/text-align-pre-wrap-trailing-spaces-multi-lines.html

Comment 8 by bugdroid1@chromium.org, Dec 20

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5a38953cb0817f29cae53c9ae16563e71da9a0b

commit f5a38953cb0817f29cae53c9ae16563e71da9a0b
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Dec 20 09:15:47 2018

[LayoutNG] Add NGLineInfo::HasTrailingSpaces()

As a follow up on review comments in r616617 (CL:1374331),
where computing the width of preserved trailing spaces became
a little more expensive, and since it's not needed in most
common cases that we wanted to add a flag to skip the
computation when it's not needed.

In most cases, NGLineBreaker knows the answer, as it breaks
lines, or removes trailing collapsible spaces. This patch
tracks it as a state, and set the result to NGLineInfo.

Bug:  913995 
Change-Id: I1eb570ea2d38bbc0597b6e03fb6dffe0d9de3973
Reviewed-on: https://chromium-review.googlesource.com/c/1382667
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618146}
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.cc
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.h
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h
[modify] https://crrev.com/f5a38953cb0817f29cae53c9ae16563e71da9a0b/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker_test.cc

Comment 9 by kojii@chromium.org, Dec 20

Status: Fixed (was: Assigned)

Sign in to add a comment