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

VERIFIED FIXED

Status

()

Core
Layout: Text
--
major
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: philippe (part-time), Assigned: roc)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 276931 [details]
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
Note:
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
Here's a reduced testcase:

test:        http://people.mozilla.com/~dholbert/tests/392435/392435-1.html
reference:   http://people.mozilla.com/~dholbert/tests/392435/392435-1-ref.html
(style file: http://people.mozilla.com/~dholbert/tests/392435/392435-style.css )

Will post as reftest patch at some point.
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.
Created attachment 278377 [details] [diff] [review]
fix

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
Status: NEW → ASSIGNED

Updated

10 years ago
Assignee: roc → nobody
Status: ASSIGNED → NEW
Component: GFX: Thebes → Layout: Fonts and Text
QA Contact: thebes → layout.fonts-and-text
Duplicate of this bug: 394146
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.
Created attachment 278886 [details] [diff] [review]
fix

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
Status: NEW → ASSIGNED
Attachment #278886 - Flags: review?(smontagu)
(Reporter)

Comment 7

10 years ago
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: http://www.yomiuri.co.jp/dy/
(Reporter)

Comment 8

10 years ago
Created attachment 279314 [details]
crash log, comment 7

With Minefield debug build; an opt Camino build gives the same stack.
(Reporter)

Comment 9

10 years ago
Created attachment 279315 [details]
test case crashes patched browser

close tab or use back button and this page crashes.
loading a page over it does not cause problems
(Reporter)

Updated

10 years ago
Attachment #279314 - Attachment mime type: application/octet-stream → text/plain
Created attachment 279396 [details] [diff] [review]
updated fix

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)
(Reporter)

Comment 11

10 years ago
(In reply to comment #10)
> Created an attachment (id=279396) [details]
> updated fix

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

Updated

10 years ago
Attachment #279396 - Flags: review?(smontagu) → review+
Attachment #279396 - Flags: approval1.9?
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?

a1.9=dbaron
Attachment #279396 - Flags: approval1.9? → approval1.9+
No, I will check in a test based on comment #1
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Checked in testcase
Flags: blocking1.9? → in-testsuite+

Comment 16

10 years ago
I confirm it fixes the problem I had on liberation.fr in bug 394146 
(Reporter)

Comment 17

10 years ago
verified with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007090717 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.