Closed Bug 406299 Opened 15 years ago Closed 14 years ago

soft-hyphen reftest failures on some systems

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(3 files)

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.
I wonder if it's because of the <br> in the soft-hyphens-1-ref.html...
Assignee: nobody → roc
Flags: wanted1.9.1?
Attached patch fixSplinter Review
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.
Attachment #329611 - Flags: superreview?(dbaron)
Attachment #329611 - Flags: review?(dbaron)
Comment on attachment 329611 [details] [diff] [review]
fix

r+sr=dbaron
Attachment #329611 - Flags: superreview?(dbaron)
Attachment #329611 - Flags: superreview+
Attachment #329611 - Flags: review?(dbaron)
Attachment #329611 - Flags: review+
Pushed 7a86c32039a2.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Do we want to get this on branch, since tests depend on it?
Flags: wanted1.9.0.x?
It's not that important IMHO, since most patches will go through trunk and be tested there.
Flags: wanted1.9.1? → wanted1.9.1+
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. Please re-nom if you disagree.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
You need to log in before you can comment on or make changes to this bug.