1478574 - spurious break immediately before an explicit hyphen when hyphens:auto is in effect

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase - observe the spurious breaks, leaving isolated hyphens

Observed while testing bug 1476437: when there is an explicit hyphen in a word, and auto-hyphenation also operates on one (or both) of the parts of the word, we incorrectly introduce an extra hyphenation break immediately -before- the explicit hyphen.

So rendering is OK for:

  data:text/html,<div lang="en" style="width:0;hyphens:auto">hard-hit

(breaks only at the explicit hyphen), but with:

  data:text/html,<div lang="en" style="width:0;hyphens:auto">hard-hitting

we correctly hyphenate "hitting", but also introduce an extra break before the existing hyphen (oops!), and end up with:

  hard-
  -
  hit-
  ting

which is clearly wrong.

This is a regression from bug 1056516.

Comment 1

[Jonathan Kew [:jfkthame]]

This is caused by an off-by-one error at https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/layout/generic/nsTextFrame.cpp#3646, where we're recording the position of an explicit hyphen. Potential line-break positions are recorded on the character -after- the potential break (the clue is in the name of the array 'aBreakBefore'), when we find a hyphen at position /i/ in the range, we should set the HyphenType::Explicit flag at /i+1/, otherwise we're recording a potential line-break -before- the hyphen character.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for interaction of explicit hyphen and auto-hyphenation in the same word

The testcase in reftest form, as we clearly don't have enough test coverage of the various edge-cases around here. :(

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Fix off-by-one error in marking explicit hyphens, so we don't introduce spurious pre-hyphen breaks

Ideally, this wants to land before bug 1476437, as this error also impacts the first of the reftests to be added there.

Comment 4

[Jonathan Kew [:jfkthame]]

Try run (together with bug 1476437):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6fcbb7728907de2775b94896dd82c7a76aea5df

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8995108 [details] [diff] [review]
Reftest for interaction of explicit hyphen and auto-hyphenation in the same word

r=dbaron, although I wonder whether maybe these should be contributed to the CSS test suite (even though you can't really guarantee what the English hyphenation rules are)...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb45de7ff9d
Reftest for interaction of explicit hyphen and auto-hyphenation in the same word. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/144a3d2b7da9
Fix off-by-one error in marking explicit hyphens, so we don't introduce spurious pre-hyphen breaks. r=dbaron

Comment 7

[Jonathan Kew [:jfkthame]]

Agreed, ideally we'd move various of these tests to the CSS suite, but to do that we'd need to consider how to test without depending on the exact breaks implemented by hyphens:auto. I'm not sure how much variability there currently is between browsers' hyphenation rules. Maybe it'd be OK to just use words where it's particularly obvious what the "only" reasonable hyphenation would be, even though the spec does not mandate following a specific dictionary or whatever...

Comment 8

[Tiberius Oros[:tiberius_oros]]

Backed out 5 changesets (bug 1478574) for failing hyphenation-control-5.html on windows builds

Backout: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a5e31e37ae9be1d1d352f50432a574ce25f50a
Push with the failures: 
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d8d3c9e7b8323246f21a8142571b99fc5f80b93b
Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=190673432&repo=mozilla-inbound&lineNumber=29204

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f98633a0e8f
Reftest for interaction of explicit hyphen and auto-hyphenation in the same word. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f907558ec7f
Fix off-by-one error in marking explicit hyphens, so we don't introduce spurious pre-hyphen breaks. r=dbaron

Comment 10

[Jonathan Kew [:jfkthame]]

The reftest failure just shows a minor DirectWrite antialiasing difference on the trailing edge of the hyphen glyph; added the necessary fuzzy-if() annotation and re-landed.

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5f98633a0e8f
https://hg.mozilla.org/mozilla-central/rev/0f907558ec7f

Comment 12

[Rares Doghi, Desktop QA]

Attachment: hard-hits_Comparison.png

Comment 13

[Rares Doghi, Desktop QA]

Hi everyone, I retested this issue in the latest version of Firefox Nightly 63.0a1 (2018-08-01), and even though it's an improvement on the old builds, it still splits differently than Chrome, can you guys take a look at the attachment and tell me if this is how its suppose to be so we can confirm it as fixed.

Comment 14

[Jonathan Kew [:jfkthame]]

Firefox is using auto hyphenation on the parts of the already-hyphenated word, while Chrome isn't.

The spec[1] says

> Automatic hyphenation opportunities within a word must be ignored if the word contains
> a conditional hyphen (­ or U+00AD), in favor of the conditional hyphen(s). However,
> if, even after breaking at such opportunities, a portion of that word is is still too
> long to fit on one line, an automatic hyphenation opportunity may be used.

so it explicitly allows this behavior.

It looks like Chrome doesn't use automatic hyphenation if the resulting word fragments still don't fit within the available width. That's kind of an odd quirk in their implementation, I think, and means that setting width:0 (as in the testcase here) doesn't work to trigger "all possible breaks".

E.g. compare Firefox and Chrome rendering for

  data:text/html,<div lang="en" style="font-family:monospace;width:5ch;hyphens:auto">supercalifragilisticexpialidocious

I'd argue the Firefox result is more sensible/useful, and should be considered correct. Indeed, given that the spec[2] says

> When wrapping is enabled (see white-space), the UA must minimize the amount of content
> overflowing a line by wrapping the line at a soft wrap opportunity, if one exists.

I believe Chrome is wrong: it is failing to use auto-hyphenation wrap opportunities to minimize the amount of overflow.


[1] https://drafts.csswg.org/css-text/#valdef-hyphens-auto
[2] https://drafts.csswg.org/css-text/#line-breaking

Comment 15

[Jonathan Kew [:jfkthame]]

FTR, I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=870219 to report Chrome's incorrect behavior.

Comment 16

[Rares Doghi, Desktop QA]

Based on the improvements from older versions and the content from Comment 14, I will mark this issue Verified as Fixed.