399531 - Soft hyphen regressions and incorrect outline on spans containing line-breaks

Status: RESOLVED FIXED

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

Description

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

Attachment: Testcase

The first issue is that I stupidly forgot to include layout/reftests/text/reftest.list in the master reftest.list so those tests weren't being run, so they regressed. In particular the soft-hyphen tests are currently broken. The problem is that we need to have TrimTrailingWhiteSpace be able to set TEXT_HYPHEN_BREAK in the text frame, if the text frame couldn't tell that it was at the end of the line during its reflow. There's a secondary problem that the reference page should use U+2010 as its hyphen since that's what the textframe code prefers.

Adding a hyphen in TrimTrailingWhitespace means we need to update our overflow area, so I added a hook for that to TrimTrailingWhiteSpace. And since I was changing that signature I took the opportunity to move it to nsTextFrame.h out of nsIFrame. This update to the overflow area also fixes an existing bug I noticed earlier where in some cases the outline for an inline element can include space for a trimmed-away space. I'll add a testcase that shows this on trunk.

Comment 1

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

Attachment: fix

This fixes it. I'll add the testcase in this bug as a reftest too.

Comment 2

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

Requesting blocking on the grounds that this is a bug in a new feature (and actually a regression that should have been picked up by reftests already).

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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

(Please wait until after review before asking for approval)

Comment 4

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

Attachment: updated to trunk

This rotted...

Comment 5

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

P1 because it fixes 402435, which is P1.

Comment 6

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

Comment on attachment 287522 [details] [diff] [review]
updated to trunk

>+  // Method that is called for a nonempty text frame that is logically

... but you're changing nsLineLayout so that it is called for empty text frames.  (Why?  When would such a text frame be trimmed?)

>+    // an amount to *subtract* from the frame's width (zero if !mTrimmed)

Not sure what !mTrimmed means, given that mTrimmed only exists as an nsTextFrame::TrimmedOffsets member of ClusterIterator.

>   if (!mTextRun)
>-    return NS_ERROR_FAILURE;
>+    return result;

How about an NS_ENSURE_TRUE so that there's some warning if this fails?

Comment 7

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

Ah, if you need to include empty text frames because those with only a soft hyphen show up as empty at that point, could you add a comment saying so?

>   // aDeltaWidth is *subtracted* from our width.
>   // If advanceDelta is positive then setting the line break made us longer,
>   // so aDeltaWidth could go negative.
>-  aDeltaWidth = NSToCoordFloor(delta - advanceDelta);
>+  result.mDeltaWidth = NSToCoordFloor(delta - advanceDelta);

What about delta being negative due to a soft-hyphen?

Comment 8

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

(In reply to comment #6)
> (From update of attachment 287522 [details] [diff] [review])
> >+  // Method that is called for a nonempty text frame that is logically
> 
> ... but you're changing nsLineLayout so that it is called for empty text
> frames.

Sorry, strike "nonempty" there.

> (Why?  When would such a text frame be trimmed?)

Hello <span> </span>

The single-space textframe will be trimmed and so will the "Hello " textframe.

> >+    // an amount to *subtract* from the frame's width (zero if !mTrimmed)
> 
> Not sure what !mTrimmed means, given that mTrimmed only exists as an
> nsTextFrame::TrimmedOffsets member of ClusterIterator.

I think I meant !mChanged there.

> >   if (!mTextRun)
> >-    return NS_ERROR_FAILURE;
> >+    return result;
> 
> How about an NS_ENSURE_TRUE so that there's some warning if this fails?

OK.

> Ah, if you need to include empty text frames because those with only a soft
> hyphen show up as empty at that point, could you add a comment saying so?

OK.

> What about delta being negative due to a soft-hyphen?

I should change the comment and the warning so they only apply when a soft hyphen is not present. Under normal circumstances, if we break at a soft hyphen, it's because we recorded a break opportunity there and at that time we will have correctly noted whether the hyphen fits in the available width or not, which will affect which potential break is recorded/chosen (see nsLineLayout::NotifyOptionalBreakPosition). So making the frame wider here should be OK. If we make the frame wider because shaping changed with the text at the end of the line, *then* we have a problem because that would not have been accounted for during reflow.

Comment 9

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

(In reply to comment #8)
> Hello <span> </span>
> 
> The single-space textframe will be trimmed and so will the "Hello " textframe.

Ah, ok, so single-space text frames are counted as empty here.

> > Ah, if you need to include empty text frames because those with only a soft
> > hyphen show up as empty at that point, could you add a comment saying so?
> 
> OK.

I'm not sure if it's even true.  Is it?

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Hello <span> </span>
> > 
> > The single-space textframe will be trimmed and so will the "Hello " textframe.
> 
> Ah, ok, so single-space text frames are counted as empty here.

Yeah, it's a "logically empty" (collapsed-away) textframe.

> > > Ah, if you need to include empty text frames because those with only a soft
> > > hyphen show up as empty at that point, could you add a comment saying so?
> > 
> > OK.
> 
> I'm not sure if it's even true.  Is it?

It's true that that's why I had to start calling TrimTrailingWhiteSpace on "logically empty" textframes.

Comment 11

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

Comment on attachment 287522 [details] [diff] [review]
updated to trunk

ok, r+sr=dbaron with the above comments

Comment 12

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

Checked in.