Bug 89342 – Style of :first-letter block isn't propagated to its inline child

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: CSS

[Marc Marshall]

If you use a :first-letter pseudoclass to apply a font referenced via an @font-face directive, and the first letter is part of a word in an italicized style (em or i), the character in question vanishes when the page is first loaded or re-loaded.

Resizing the window will cause the missing drop cap to immediately reappear, while doing a reload after it has reappeared will cause it to disappear again.  Checked release Safari, current Nightly, and release Chrome--all have the same behavior.

Notably, it doesn't matter whether the external font file even exists; the character vanishes whether it is being rendered in the externally loaded font, or a system-installed fallback. It also appears that the character really isn't being rendered, since if margins are set on the drop cap the empty gap visibly changes size when the page is resized and the character reappears.

Adding an explicit :first-letter declaration for the italicized version causes the character not to disappear.  URL referenced is a minimum test case that I came up with.

[Marc Marshall]

Created attachment 148121 [details]
Simplified testcase; note that the testcase does not include the external font, but the bug is triggered whether it exists or the fallback is used.

[Daniel Bates]

Created attachment 152170 [details]
Work-in-progress patch

Needs change log.

[Marc Marshall]

The bug title was just changed to say that the bug is only triggered when the custom @font fails to load, but that is not actually correct--so far as I could tell in testing, the :first-letter is not drawn even if the custom @font DOES load.

I assume the patch is the same either way, so it is probably irrelevant.

[Daniel Bates]

Created attachment 159978 [details]
Patch and layout tests

I chose to use pixel tests as opposed to ref tests since I couldn't think of a good way to write ref tests that wouldn't go through a similar code path. I am open to suggestions.

[Eric Seidel (no email)]

I see the first-letter intermittently on reload.  Is this a lack-of-layout problem or lack-of-repaint problem?

[WebKit Review Bot]

Comment on attachment 159978 [details]
Patch and layout tests

Attachment 159978 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13568328

New failing tests:
fast/css/first-letter-inline-inherits-font-size-and-non-existent-custom-font.html
fast/css/first-letter-inline-inherits-font-size-and-non-existent-custom-font2.html

[WebKit Review Bot]

Created attachment 159984 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Daniel Bates]

Comment on attachment 159978 [details]
Patch and layout tests

Clearing review flag to further look into this patch.

[Daniel Bates]

(In reply to comment #5)
> Is this a lack-of-layout problem or lack-of-repaint problem?

Neither. We never propagate style changes from the first-letter's containing block B to its inline child B_c (see the code in RenderBlock::updateFirstLetter() below the comment that starts with "Drill into inlines..." for how we pick this child). Without loss of generality, when the style of B changes, say its custom font successfully loads or fails to load, then we never update the style of B_c.

Additional remarks:

After creating the first-letter element P_f (a RenderTextFragment object) with the style of its containing block, we never propagate to P_f subsequent style changes to its containing block. Let P be the containing block of P_f and S_p be the style (RenderStyle) of P. Suppose P defines the :first-letter pseudo element with a custom font c that is being loaded and we create a RenderTextFragment P_f that inherits the style of P. Then "P_f's style" := S_f == S_p; => S_f's font is being loaded (*). When c has been successfully loaded or fails to load then we update the style of P, call this style S_p' != S_p. Notice that the font of S_p' has been loaded (i.e. font().loadingCustomFonts() returns true). But S_p' != S_f. And S_f's font hasn't been loaded by (*) (i.e. font().loadingCustomFonts() return false). Therefore, we don't draw the text for P_f to the screen by <http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/Font.cpp?rev=125766#L145>.

[Daniel Bates]

(In reply to comment #9)
> (In reply to comment #5)
> > Is this a lack-of-layout problem or lack-of-repaint problem?
> 
> ... We never propagate style changes from the first-letter's containing block B...

I meant to write:

We never propagate style changes from the first-letter block B...

[Elliott Sprehn]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > Is this a lack-of-layout problem or lack-of-repaint problem?
> >  ..

After much discussion about this with eseidel it looks like this patch is fine. We already call updateFirstLetter() from inside styleDidChange over in RenderTextFragment. If we added generated content we'd do a layout and then update the first letter again.

There's a simpler way to see this bug in the new attached reduction.

[Elliott Sprehn]

Created attachment 160055 [details]
Reduction

[Elliott Sprehn]

Comment on attachment 159978 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=159978&action=review

> LayoutTests/fast/css/first-letter-inline-inherits-font-size-and-non-existent-custom-font2.html:4
> +@font-face {

Use a ref test instead with the new attached reduction, don't use a pixel test. It should be trivial to change it to always have the big red text for the -expected.html

[Abhishek Arya]

I think why don't we just call updateFirstLetter as the last statement in RenderBlock::styleDidChange [when updateBeforeAfterContent is done too] and try to yank the line out from RenderBlock::layout. we always wanted to do this change so that renderers are not destroyed during layout AND also prevent double updating of first-letter [one before generated content and one after generated content]. RenderBlock::computePreferredLogicalWidths might be step 2 in the removal where we also call updateFirstLetter.

[Abhishek Arya]

See also https://bugs.webkit.org/show_bug.cgi?id=94850. It is pretty high time that we need to move updateFirstLetter from inside layout. While discussing this with Daniel, i think there are three hooks needed.

1. Remove it from RenderBlock::layout.
2. Add updateFirstLetter it to the end of RenderBlock::styleDidChange, RenderBlock::addChild, RenderBlock::removeChild.
3. Add removeOldFirstLetter to start of RenderBlock::styleDidChange [since we need to do it before updating generated content], RenderBlock::addChild, RenderBlock::removeChild. removeOldFirstLetter is important that we don't have in our current code at all.

RenderBlock::addChild can be optimized a lot by not doing anything if !newChild->isText() and some more conditions like beforeChild 0 with multi-children, etc.
RenderBlock::removeChild is a little tricky since text child can be underneath the newChild

[Brent Fulgham]

WebKit and Blink handle this properly. Firefox fails.

[Radar WebKit Bug Importer]

<rdar://problem/96959497>