Closed Bug 392435 Opened 12 years ago Closed 12 years ago

::first-line and ::first-letter - font-size and font-weight is propagated to all lines of affected block.


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

Not set





(Reporter: phiw2, Assigned: roc)



(Keywords: regression, testcase)


(4 files, 2 obsolete files)

Attached file test case
Testcase: a paragraph <p> with p::first-letter {font-size:200%;}, a div with div::first-line {font-weight:bold;}. 
The whole block uses the property declared on the :first-line or ::first-letter

Also happens with font-variant:small-caps, probably other text properties.

regressed between:
20070815_1129_firefox-3.0a8pre.en-US.mac (works)
20070815_1159_firefox-3.0a8pre.en-US.mac (fails)

--> bug 385270
If one switches tabs, and wait a few moments, then switch back to the tab with the testcase, the styling is applied correctly in case of the first-letter, but the line-wrapping is not correct.
Flags: blocking1.9?
OS: Mac OS X → All
Hardware: Macintosh → All
Also, it looks like thie affects all font-* properties, not just font-weight and font-style.
(I've tried font-weight, font-style, font-size, font-variant, and font-family)

Other attributes like color, background, and text-decoration don't seem to be affected.
Attached patch fix (obsolete) — Splinter Review
This patch fixes it, but I need to think about whether it's the right patch.

A better way might be to just test whether the textruns are equal and the style contexts are equal.
Assignee: nobody → roc
Assignee: roc → nobody
Component: GFX: Thebes → Layout: Fonts and Text
QA Contact: thebes → layout.fonts-and-text
It's supposed to work like this:
-- A nsFirstLetterFrame wraps the entire text (nsTextFrame)
-- the nsTextFrame reflows, creates a textrun for the entire text, but ends its reflow after the first letter
-- the nsFirstLetterFrame creates a continuation for the nsTextFrame; the continuation has the same style context as the first textframe, so it takes a reference to the first textframe's text run
-- the nsFirstLetterFrame puts that continuation on its overflow list
-- the block or containing inline creates a continuation for the nsFirstLetterFrame, and reflows it
-- The continuation nsFirstLetterFrame does a DrainOverflowFrames which pulls the textframe from the first nsFirstLetterFrame's overflow list
-- nsFirstLetterFrame::DrainOverflowFrames does a SetStyleContext on the pulled child to give it non-first-letter style
-- That calls nsTextFrame::DidSetStyleContext which clears the frame's textrun
-- Then we reflow the continuation textframe, which recreates the textrun
-- Recreating the textrun notices that the style context, and indeed font style, changes between the first text frame and its continuation and starts a new textrun for the continuation

Some step along here is broken, not sure which one yet.
Attached patch fix (obsolete) — Splinter Review
The problem is that ClearTextRun has a precondition that I'd basically forgotten about: it does nothing if the frame is not one of the "textrun owners" --- the frames that are the heads of next-in-flow-chains associated with the textrun. In the cases here we're calling DidSetStyleContext() on frames that aren't owners, so they don't clear out their text runs and we use stale bad ones.

Removing that precondition allows ClearTextRun to work no matter which frame you call it on. This makes a lot more sense. I think I originally set it up with the owner check because I was concerned about O(N^2) performance tearing down a list of textframe continuations, but this isn't actually a problem since the first ClearTextRun to a frame in the chain will null out all the mTextRun pointers (which is O(N), but that's unavoidable) and all subsequent ClearTextRuns will exit immediately since mTextRun is null.
Assignee: nobody → roc
Attachment #278377 - Attachment is obsolete: true
Attachment #278886 - Flags: review?(smontagu)
So. I had a crash with this patch (OS X 10.4.10 ppc).
A very simple test file, p::first-letter {font-size:2em;}
Leaving the page by either closing the tab or using the back button would hang and then crash the browser:

0   libthebes.dylib          	0x062956c0 gfxTextRun::~gfxTextRun [in-charge deleting]() + 120 (gfxFont.h:419)
1   libthebes.dylib          	0x062956bc gfxTextRun::~gfxTextRun [in-charge deleting]() + 116 (gfxFont.cpp:735)
2   libgklayout.dylib        	0x07ba1b54 nsContinuingTextFrame::Destroy() + 56 (nsTextFrameThebes.cpp:3359)
3   libgklayout.dylib        	0x07b67754 nsLineBox::DeleteLineList(nsPresContext*, nsLineList&) + 124 (nsLineBox.cpp:361)

With a debug Minefield build, I see this:
firefox-bin(11650,0xa000ed88) malloc: ***  Deallocation of a pointer not malloced: 0x26c6600; This could be a double free(), or free() called with the middle of an allocated block; Try setting environment variable MallocHelp to see tools to help debug
(didn't try the Malloc suggestion, don't understand any of it...:)
Full crash log and testcase coming.

Live URL:
Attached file crash log, comment 7
With Minefield debug build; an opt Camino build gives the same stack.
close tab or use back button and this page crashes.
loading a page over it does not cause problems
Attachment #279314 - Attachment mime type: application/octet-stream → text/plain
Attached patch updated fixSplinter Review
Thanks for catching that. We need to call ClearTextRun before unlinking the continuation chain.
Attachment #278886 - Attachment is obsolete: true
Attachment #279396 - Flags: review?(smontagu)
Attachment #278886 - Flags: review?(smontagu)
(In reply to comment #10)
> Created an attachment (id=279396) [details]
> updated fix

Yup, this one does what it says. No problems anymore.

Attachment #279396 - Flags: review?(smontagu) → review+
Comment on attachment 279396 [details] [diff] [review]
updated fix

>? layout/reftests/first-letter/available-width-1.html
>? layout/reftests/first-letter/available-width-ref.html
>? layout/reftests/first-letter/cluster-1-ref.html
>? layout/reftests/first-letter/cluster-1.html
>? layout/reftests/text/white-space-1.html
>? layout/reftests/text/white-space-1d.html

Are these tests that you should be checking in?

Attachment #279396 - Flags: approval1.9? → approval1.9+
No, I will check in a test based on comment #1
checked in
Closed: 12 years ago
Resolution: --- → FIXED
Checked in testcase
Flags: blocking1.9? → in-testsuite+
I confirm it fixes the problem I had on in bug 394146 
verified with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007090717 Minefield/3.0a8pre
You need to log in before you can comment on or make changes to this bug.