441418 - white-space:pre with first-letter makes content disappear

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Attachment: testcase #2

See attached testcase.

The problem is that nsBlockFrame::ReflowInlineFrame tries to guess whether an incomplete reflow status is because of a first-letter ending, and decides whether to start a new line based on that. In this testcase the preformatted newline line break is treated as the end of the first-letter, and nsBlockFrame fails to start a new line; in the ensuing chaos the rest of the content is not reflowed.

Comment 1

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

Attachment: fix

Don't guess, just add a new reflow status bit to indicate when a break occurred due to a first-letter frame ending.

Comment 2

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

Note to self: may need to update patch based on changes in bug 399941.

Comment 3

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

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

> #ifdef NOISY_FIRST_LETTER
>+  PRBool reflowingFirstLetter = aLineLayout.GetFirstLetterStyleOK();

Could you put the contents of the ifdef inside {} so that variables don't leak out?

>diff -NrpU12 mozilla-trunk.f0aec487ed11/layout/reftests/bugs/441418-1-ref.html mozilla-trunk/layout/reftests/bugs/441418-1-ref.html
>+<p><span style="font-size:200%"><br></span>

I'm not really sure this is justified or that we expect the behavior of this to be stable.  But I guess we'll deal with that if we change it...

It would also be good to add a test for the case where the wrapping is just normal wrapping at a space rather than wrapping due to 'pre'.

The reftests could also go in layout/reftests/first-letter/ if you want.

r+sr=dbaron, although I don't completely follow it

Comment 4

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

(In reply to comment #3)
> Could you put the contents of the ifdef inside {} so that variables don't leak
> out?

Sure

> It would also be good to add a test for the case where the wrapping is just
> normal wrapping at a space rather than wrapping due to 'pre'.

OK

> The reftests could also go in layout/reftests/first-letter/ if you want.

OK.

> r+sr=dbaron, although I don't completely follow it

Which part is hard?

Comment 5

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

Pushed 2bafc1749899.