[LayoutNG] Issue with pre-wrap inside NGLineBreaker(?) and floats. |
|||
Issue descriptionSee 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.
,
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.
,
Dec 12
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.
,
Dec 12
LGTM
,
Dec 13
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
,
Dec 13
,
Dec 14
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
,
Dec 20
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
,
Dec 20
|
|||
►
Sign in to add a comment |
|||
Comment 1 by kojii@chromium.org
, Dec 12Owner: kojii@chromium.org
Status: Assigned (was: Untriaged)