Closed Bug 1472386 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: fantasai.bugs, Assigned: xidorn)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file testcase (obsolete) —
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.
Add this
Attached file actual testcase (obsolete) —
Attachment #8988956 - Attachment is obsolete: true
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.
Flags: needinfo?(xidorn+moz)
Priority: -- → P3
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
Nice "test" to check https://jsfiddle.net/ofgd83um/54/
(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.
Attachment #8989738 - Attachment is obsolete: true
Attachment #8991875 - Attachment mime type: text/plain → text/html
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">तपाईंको नाम के हो?
Attachment #8990894 - Flags: review?(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/
Attachment #8990894 - Flags: review?(jfkthame) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f52422f008ed
Take overflow-wrap into account when calculating min-content intrinsic size. r=jfkthame
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12002 for changes under testing/web-platform/tests
https://hg.mozilla.org/mozilla-central/rev/f52422f008ed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1476180
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"?
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.
@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.
Depends on: 1478455
Filed w3c/csswg-drafts#2951 for this change doesn't seem to be web-compatible.
Depends on: 1479302
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
Blocks: 1480284
Depends on: 1480358
See Also: → 1505786
You need to log in before you can comment on or make changes to this bug.