1476437 - auto-hyphenation ('hyphens: auto') hyphenates in the wrong place following a hyphen and whitespace collapsing

Status: RESOLVED FIXED

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

Description

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

Attachment: simplified testcase

I noticed this in the wild on https://drafts.csswg.org/css-logical-1/#logical-shorthand-keyword , which is probably one of a pretty small number of sites that use 'hyphens: auto'.  In particular, what I observed was hyphenation as follows:


first line:
When the logical keyword is present in the value, the values that follow are assigned to the flow-r-

second line:
elative properties as follows:

I simplified the testcase, and it seems like the key characteristic of the bug is that auto-hyphenation is happening at the incorrect position when it happens in an already hyphenated word that follows white-space collapsing.  In the original, "flow-relative" is preceded by a CR and two spaces.  Replacing the CR with a space (i.e., three spaces total) produces the same result, hyphenating as "flow-r-elative"; using four spaces leads to "flow-re-lative".

Comment 2

[Jonathan Kew [:jfkthame]]

Looks like this is a regression from bug 1056516. (Before that, we'd auto-hyphenate the second part of the already-hyphenated word, which we're not really supposed to do, but at least we'd do it in the right place.)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for interaction between whitespace collapsing, explicit hyphen, and auto-hyphenation

Here's a reftest based on your testcase, which fails (hugely) with current trunk code.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled

And this fixes the indexing into the original text when we're checking for explicit hyphens, which is what's going wrong here.

Comment 5

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

Comment on attachment 8993301 [details] [diff] [review]
Reftest for interaction between whitespace collapsing, explicit hyphen, and auto-hyphenation

Letting all of the test items flow indepeendently between balanced columns is a bit gutsy, but it seems to work!

Comment 6

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

Comment on attachment 8993303 [details] [diff] [review]
Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled

Is there a simple explanation for why it's OK to throw away whatever the current position in mStart was?  At least, it looks to me like that's what ConvertSkippedToOriginal does... which makes it seem like this is making two changes: (a) throwing away the initial position in mStart and just working from the start of the string (b) considering skipped characters after the beginning.  Is it assumed that mStart is currently at a skipped offset of 0?

Comment 7

[Jonathan Kew [:jfkthame]]

Ah - you're right, this isn't quite correct; it works for the simple testcase here but may fail with more extensive examples, as it doesn't correctly check for explicit hyphens in continuations when mStart.GetSkippedOffset() was non-zero. We should be doing ConvertSkippedToOriginal(aRange.start + i), just like in the call to CanHyphenateBefore() a few lines below.

I'll attach an additional reftest that exercises this; with the original patch here, it (correctly) gives the same result with or without collapsed whitespace, but the result (in both cases) incorrectly does auto-hyphenation in some cases where it should be inhibited because of an explicit hyphen in the word.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Further reftest for multi-line content with whitespace/explicit-hyphen/autohyphenation

Further reftest; this fails on current trunk, and still fails (somewhat differently) with the original patch above.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled

And here's the updated patch that makes both reftests pass as expected.

Comment 10

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

Comment on attachment 8994836 [details] [diff] [review]
Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled

OK, the argument that this matches the CanHyphenateBefore call a few lines below seems reasonably convincing.

It still seems like there's still an additional difference relative to the old code, which is that it seems like aRange is required to be a *subrange* of mStart to mLength (but not necessarily the whole thing), and if it doesn't start at mStart then the new behavior seems different as well.

I still don't follow the whole thing -- it's rather complicated, and it feels like it would be nice if these various offsets were documented somewhere...

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17db916959e
Reftest for interaction between whitespace collapsing, explicit hyphen, and auto-hyphenation. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/280aff154491
Further reftest for multi-line content with whitespace/explicit-hyphen/autohyphenation. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d3c9e7b832
Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled. r=dbaron

Comment 12

[Tiberius Oros[:tiberius_oros]]

Backed out 5 changesets (bug 1476437) 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 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1cdd4b96b3
Reftest for interaction between whitespace collapsing, explicit hyphen, and auto-hyphenation. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb04354c2cf
Further reftest for multi-line content with whitespace/explicit-hyphen/autohyphenation. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05794e50453
Correct original-text indexing when checking for explicit hyphens in text where auto-hyphenation is enabled. r=dbaron

Comment 14

[Jonathan Kew [:jfkthame]]

The failure was from the reftest in bug 1478574, pushed at the same time as this. Re-landed.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/af1cdd4b96b3
https://hg.mozilla.org/mozilla-central/rev/fbb04354c2cf
https://hg.mozilla.org/mozilla-central/rev/a05794e50453

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Seems like we should probably just let this ride the trains.