1472386 - overflow-wrap/word-wrap: break-word should affect intrinsic (min-content) sizing

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

[fantasai]

Attachment: testcase

One of the core frustrations that authors run into with `auto` and `fr` sizing in Grid (and with the `min-width: auto` behavior of Flexbox) is cases where they've already handled overflow in some other way (such as scrollbars, or more aggressive line-breaking allowances), but the box insists on being wider.

One of these cases was resolved in https://github.com/w3c/csswg-drafts/issues/2682 - the word-wrap/overflow-wrap property is now defined to affect min-content sizing, just like any other line-breaking control.

Comment 1

[idontlikespying]

Add this

Comment 2

[fantasai]

Attachment: actual testcase

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

fantasai told me this should be considered as a webcompat issue because otherwise we may end up needing to implement some nonsense just because WebKit (before the fork) happened to do something which web authors want in a wrong way. I'm going to look into this after the meetings.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1472386 - Take overflow-wrap into account when calculating min-content intrinsic size.

Review commit: https://reviewboard.mozilla.org/r/255896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255896/

Comment 5

[idontlikespying]

Nice "test" to check https://jsfiddle.net/ofgd83um/54/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to idontlikespying from comment #5)
> Nice "test" to check https://jsfiddle.net/ofgd83um/54/

It seems the patched build work as expected for this test.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: fantasai's testcase, with typo fixed

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8990894 [details]
Bug 1472386 - Take overflow-wrap into account when calculating min-content intrinsic size.

https://reviewboard.mozilla.org/r/255896/#review263670

::: gfx/thebes/gfxTextRun.cpp:1252
(Diff revision 1)
> +    for (uint32_t i = ligatureRange.start; i < ligatureRange.end; ++i) {
> +        gfxFloat width = GetAdvanceForGlyph(i);
> +        result = std::max(result, width);
> +    }

I think this should really iterate and measure by clusters rather than by characters.

E.g. try an example like

    data:text/html;charset=utf-8,<div style="width:0;"><div style="float:left;word-wrap:break-word;border:1px solid red">तपाईंको नाम के हो?

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8990894 [details]
Bug 1472386 - Take overflow-wrap into account when calculating min-content intrinsic size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255896/diff/1-2/

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8990894 [details]
Bug 1472386 - Take overflow-wrap into account when calculating min-content intrinsic size.

https://reviewboard.mozilla.org/r/255896/#review263962

::: gfx/thebes/gfxTextRun.cpp:1239
(Diff revision 2)
>  }
>  
> +gfxFloat
> +gfxTextRun::GetMinAdvanceWidth(Range aRange)
> +{
> +    NS_ASSERTION(aRange.end <= GetLength(), "Substring out of range");

I think we generally prefer MOZ_ASSERT when adding new assertions.

::: gfx/thebes/gfxTextRun.cpp:1250
(Diff revision 2)
> +        ComputePartialLigatureWidth(Range(aRange.start, ligatureRange.start),
> +                                    nullptr),
> +        ComputePartialLigatureWidth(Range(ligatureRange.end, aRange.end),
> +                                    nullptr));
> +
> +    // XXX Do we need to take spacing into account? When each graphme cluster

s/graphme/grapheme/

::: testing/web-platform/tests/css/css-text/overflow-wrap/overflow-wrap-min-content-size-002.html:9
(Diff revision 2)
> +<link rel="author" title="Xidorn Quan" href="https://www.upsuper.org/">
> +<link rel="author" title="Mozilla" href="https://www.mozilla.org/">
> +<link rel="help" href="https://drafts.csswg.org/css-text-3/#overflow-wrap-property">
> +<meta name="flags" content="">
> +<link rel="match" href="reference/overflow-wrap-min-content-size-002-ref.html">
> +<meta name="assert" content="overflow-wrap:break-word don't break graphme cluster and min-content intrinsic size should take that into account.">

s/don't/doesn't/
s/graphme/grapheme/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8990894 [details]
Bug 1472386 - Take overflow-wrap into account when calculating min-content intrinsic size.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255896/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f52422f008ed
Take overflow-wrap into account when calculating min-content intrinsic size. r=jfkthame

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12002 for changes under testing/web-platform/tests

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f52422f008ed

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 16

[Ronald Tilby]

I have found two cases (so far) in production web pages where this patch has caused previously horizontal text to be displayed vertically.  
The testcase for this bug shows 8 "FAIL"s when displayed in MS Edge, Google Chrome Canary Version 70.0.3500.0 (Official Build) canary (64-bit), and Google Chrome Version 68.0.3440.68 (Official Build) beta (64-bit).

Will web developers who experience adverse results of this patch have a clear path back to cross-browser compatibility?

Is this a case where the "cure" is worse than the "disease"?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

If you see more examples, please file bugs blocking this bug, or use webcompat.com.

There are two possible ways to get cross-browser compatibility:
1. push other browsers to change this
2. if there is enough site broken by this, bring it back to the CSS working group and reverse the resolution

It's not clear which is better.

As I mentioned in bug 1476180, having that bug alone isn't very convincing to reverse the resolution, but if there are more, we may reconsider.

Comment 18

[idontlikespying]

@Ron
This is proper behavior to overflow-wrap: break-word to calculate min-content. Maybe this is no place for that but there is Chromium issue (https://bugs.chromium.org/p/chromium/issues/detail?id=862127). It prority 3 feature already. Fix your code with proper overflow-wrap value.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Filed w3c/csswg-drafts#2951 for this change doesn't seem to be web-compatible.

Comment 20

[idontlikespying]

We should add `table { word-wrap: normal; }` to the UA style sheet or just not inherit that rule for tables by default from higher CSS levels