406299 - soft-hyphen reftest failures on some systems

Status: RESOLVED FIXED

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

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The tests in layout/reftests/text

== soft-hyphens-1a.html soft-hyphens-1-ref.html
== soft-hyphens-1b.html soft-hyphens-1-ref.html
== soft-hyphens-1c.html soft-hyphens-1-ref.html

Are failing on some machines. The problem appears to be that the ascent and/or descent of the soft hyphen (after font selection) is not being included in the text frame's metrics. I'm marking these tests random while I figure out what to do.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: failed soft-hyphens-1a.html

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: reference

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I wonder if it's because of the <br> in the soft-hyphens-1-ref.html...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

The problem is that we weren't adding the ascent and descent of the soft hyphen into our metrics. This causes problems when the fallback font for the Unicode hyphen character has a higher ascent or lower descent than the DOM text.

Most of the fix is simply making AddCharToMetrics call RunMetrics::CombineWith instead of feebly trying to do its own thing. There are a couple of trickier issues though:

-- text consisting of nothing but a displayed soft hyphen was being treated as if it was entirely collapsed, causing its ascent and descent to be ignored later. I'm fixing this by making a displayed soft hyphen count as "uncollapsed characters".

-- The code that zeroes out the text ascent and descent when there are no chars displayed needs to not do that when there's a soft hyphen displayed.

-- If the soft hyphen was the last character in the text, we might break the line at the start of the next text frame (because nsLineLayout::CanPlaceFrame returns false for that next text frame, because it notices that we've gone over the available width and there's a usable line break opportunity already registered). In that case we relied on TrimTrailingWhitespace to reach this frame and trigger the display of the soft hyphen. But that's no good here because it's too late to change our ascent/descent at that point.

So instead, allow (in fact, force) the following text frame(s) to be placed and rely on the second reflow of the line to force a break at the best saved line break opportunity. Then we can detect that the line wants to break at our trailing soft hyphen during Reflow.

Comment 5

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

Comment on attachment 329611 [details] [diff] [review]
fix

r+sr=dbaron

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 7a86c32039a2.

Comment 7

[Simon Montagu :smontagu]

Do we want to get this on branch, since tests depend on it?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not that important IMHO, since most patches will go through trunk and be tested there.

Comment 9

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. Please re-nom if you disagree.