8253 - Can't dynamically add ::first-line / ::first-letter style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Peter Linss]

Try:
<style>
p:hover:first-line { color: red }
</style>
<p>enough text to cause line wrapping should be here to illustrate first-line
style</p>

Note that :first-line style does not apply when the <p> gets hovered over.
If you add a non-dynamic style rule for :first-line, "p:first-line { color:
green }" then it works as expected.

The problem here is that the BlockFrame code is not looking for new :first-line
style contexts during ReResolveStyleContext. It needs to detect them and then
re-resolve children as appropriate.

Comment 1

[kipp]

Changing to M9.

Comment 2

[Christian Wenz]

Attachment: test case (as attachment for convenience)

Comment 3

[Christian Wenz]

OVERVIEW DESCRIPTION:
dynamically adding styles first-letter, first-line does not work

STEPS TO REPRODUCE:
1) Load the attachment
2 [details] [diff] [review]) If text fits in one line, resize the window so that there is a line break
3) move the mouse over the text

ACTUAL RESULT:
text is in black

EXPECTED RESULT: first line of text is in red

BUILD DATE & PLATFORM: M7, 1999070917 build, Win32

ADDITIONAL INFORMATION: p:first-line {color:red} works; p:hover:first-line
{color:red} doesn't. For a more technical information, see [e-mail]'s
comment.
Changing bug status to "[TESTCASE] ..." standard.

Comment 4

[kipp]

Dynamic addition of first-line style now works; there is zero support for
dynamic first-letter (see bug #6059).

Comment 5

[kipp]

Since you are working on moving ReResolveStyle into the frame construction
code...

Comment 6

[Peter Linss]

Dynamic addition of ":before" and ":after" pseudo elements doesn't work either.

Comment 7

[Peter Linss]

Attachment: Testcase for dynamc ":before"

Comment 8

[Pierre Saslawsky]

Reassigning peterl's bugs to myself.

Comment 9

[Pierre Saslawsky]

Accepting peterl's bugs that have a Target Milestone

Comment 10

[Pierre Saslawsky]

Pushing my M15 bugs to M16

Comment 11

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 12

[Pierre Saslawsky]

Latered.

Comment 13

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 14

[Hixie (not reading bugmail)]

Reopening and moving to Future...

Comment 15

[Mike McCaughan]

Attachment: Inclusion of :first-line oddity.

Adding feul to the fire...

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is not a problem for stylesheet switching too, not just dynamic stuff,
since switching just reresolves style....

Comment 17

[Boris Zbarsky [:bzbarsky]]

er, "This is now a problem"

Comment 18

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

Assigning pierre's remaining Style System-related bugs to myself.

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** Bug 82289 has been marked as a duplicate of this bug. ***

Comment 20

[Boris Zbarsky [:bzbarsky]]

*** Bug 193253 has been marked as a duplicate of this bug. ***

Comment 21

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

*** Bug 231205 has been marked as a duplicate of this bug. ***

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows the first-line problem more clearly

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows the first-letter problem more clearly

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Real first-letter testcase

Comment 25

[Giuseppe Bilotta]

This bug surfaces also in other situations: for example when JavaScript code assigns a new id or class to an existing tag, thus bringing such a tag under the scope of a :first-line or :first-letter CSS rule.

Attaching test-case for :first-line

Comment 26

[Giuseppe Bilotta]

Attachment: Testcase showing that changing the class of an element applies new CSS rules excpet for :first-line

This testcase shows that when JavaScript changes the class of an element, that element takes over the new styling from the CSS, except for the rules concerning :first-line ; similar results are obtained by using :first-letter, or by changing the id and using id-specific CSS rules.

Comment 27

[Phil Ringnalda (:philor)]

*** Bug 347296 has been marked as a duplicate of this bug. ***

Comment 28

[Boris Zbarsky [:bzbarsky]]

So this might not be _too_ bad to fix for first-letter now that we have a bit for it, except it would be kinda slow.  :(

Comment 29

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

See also bug 379799.

Comment 32

[Timothy Nikkel (:tnikkel)]

Attachment: patch

In the first-letter case, because we have the first letter style bit all we really need to do is set that bit and somehow make sure RecoverLetterFrames gets called on the block. Any ContentInserted on a child of the block would suffice, but I can't think of a way to make that work well. So I guess we just have to reconstruct the block.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412154 [details] [diff] [review]
patch

Several issues here:

1)  It might be better to deal with the styles going away by re-probing while reresolving those frames like we do for :before/:after.  Not sure it actually is, though.

2)  You need to add more conditions here, similar to the ones for :before/:after.  Otherwise, various anonymous blocks that correspond to nodes that happen to have first-* style will trigger this... One issue is table cells.  Do those get first-letter/first-line styling?

Also, shouldn't this check only happen for the first continuation?

Comment 34

[Timothy Nikkel (:tnikkel)]

(In reply to comment #33)
> (From update of attachment 412154 [details] [diff] [review])
> Several issues here:
> 
> 1)  It might be better to deal with the styles going away by re-probing while
> reresolving those frames like we do for :before/:after.  Not sure it actually
> is, though.

Seems like it would be a wash. But having the code for adding/removing together in one place seems like it would be a plus clarity-wise.

> 2)  You need to add more conditions here, similar to the ones for
> :before/:after.  Otherwise, various anonymous blocks that correspond to nodes
> that happen to have first-* style will trigger this... One issue is table
> cells.  Do those get first-letter/first-line styling?

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6331 says which anon boxes can have first-letter. With the exception of fieldsets (see http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9248), I would assume that first-line would be the same.

> Also, shouldn't this check only happen for the first continuation?

Ah, yes it should, the first-line check is only valid on the first continuation for one thing.

Comment 35

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Updated based on comments.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Hmm.  That assert in blockframe actually looks like it might be wrong.  Specifically, we allow first-letter/line on <html:button style="display: block">, it looks like...  I wonder whether that's new or whether we've been doing it for a while.

It also looks like we might allow it for <select>; that's only saved by the fact that we typically have no inline kids of <select>.  We should turn that off.

It also looks like we allow these styles on the block of an <svg:foreignobject>, which has a pseudo that's not in that assert's whitelist.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Seems like it would be a wash. But having the code for adding/removing together
> in one place seems like it would be a plus clarity-wise.

It's not quite a wash in cases where there are lots of first-letters/first-lines, since in those cases we end up reresolving the style context for those pseudo-elements twice.  But I think those cases are likely pretty rare.

Want to make that "do I have a first-line" check into a method we could reuse in the frame constructor where we currently probe for first-line stuff using styles?

Comment 38

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

This patch uses the frame tree check for first-line instead of probing for pseudo style in frame construction.

I made CanBeFirstLineContainer/CanBeFirstLetterContainer as complete as I can think of, including svg foreign objects, selects, and buttons.

I turned off special block styles for select frames, and call CanBeFirstLineContainer/CanBeFirstLetterContainer during frame construction so we don't try to construct first-letter/line in weird cases.

I realized that the "do I have a first-line" check had to be a bit more complicated to deal with the case of no leading inlines, and the only part that could be factored out is the "is first child a line frame with first-line pseudo style", but I didn't bother since it is just two lines. I can still factor it out if you'd like.

As in bug 484400, the first-letter tests can't use a fake first-letter styled span because they fail on Windows and Mac.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 413487 [details] [diff] [review]
patch v3

roc agreed to look at this to help release some pressure in bz's review queue.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I would actually really like to at least glance at this before checkin, once roc's ok with it.

Comment 41

[Timothy Nikkel (:tnikkel)]

There's absolutely no hurry on this bug...

Comment 42

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

+    // We check if there is an existing first-line child. If there isn't

This is all a new optimization, right? Might be better to separate the optimization from the bug fix here. How sure are we that this is a worthwhile optimization?

+    nsIAtom* frameType = aFrame->GetType();
+
+    if (!(aMinChange & nsChangeHint_ReconstructFrame)) {
+      // Check if we need to reconstruct for added/removed first-letter style,
+      // but only on the first continuation of block frames.
+      if (frameType == nsGkAtoms::blockFrame &&
+          !aFrame->GetPrevContinuation()) {
+        nsBlockFrame* blockFrame = nsLayoutUtils::GetAsBlock(aFrame);

Instead of calling GetType here I'd just call GetAsBlock and check for null, hoisted out so we can reuse the result.

In nsFrameManager::ReResolveStyleContext I'm not sure whether we should check CanBeFirstLetterContainer on the block before we probe for pseudostyle and check NS_BLOCK_HAS_FIRST_LETTER_STYLE. Since most blocks can be first-letter containers (I guess) it's probably better to probe for first-letter first. Similar for first-line. And maybe refactor the code so that we just have NeedsReconstructionForFirstLetter/NeedsReconstructionForFirstLine helper functions?

Comment 43

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Disable first-letter and first-line styles in select elements.

Comment 44

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Allow first-letter and first-line styles to be applied dynamically.

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Optimize checking for existing first-line style in frame constructor.

Comment 46

[Timothy Nikkel (:tnikkel)]

Ready for review, not sure who wants to review what though.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445527 [details] [diff] [review]
Part 1. Disable first-letter and first-line styles in select elements.

r=bzbarsky

Comment 48

[Boris Zbarsky [:bzbarsky]]

Thoughts on part 2 (and related):

1)  NS_BLOCK_FLAGS_MASK should just be fixed to exclude the bits we exclude in
    nsBlockFrame::Init.
2)  It'd be really nice if we could use flags to implement CanBeFirstContainer
    (which CSSFC could then set to match what it thinks it's doing), but we only
    have one free flag, right?  Could use that one flag for everything except
    fieldsets?  Why are we not allowing first-letter on fieldsets anyway?
3)  Just pass nsStyleContext* instead of the nsRefPtr ref to the static inlines
    in the frame manager?

Comment 49

[Boris Zbarsky [:bzbarsky]]

Haven't looked at part 3 yet, pending us sorting out part 2.

Comment 50

[Boris Zbarsky [:bzbarsky]]

And in particular, I'd want to see some perf data on part 3.

Comment 51

[Timothy Nikkel (:tnikkel)]

Pushed part 1
http://hg.mozilla.org/mozilla-central/rev/b2d0bd7761a1

Comment 52

[Timothy Nikkel (:tnikkel)]

With bug 570837 we'll have 64 bits of frame state so we might even want to use a bit for "has first line style" and avoid the messy frame tree check.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Timothy, you want to pick this up again now that we have free flag bits?

Comment 54

[Timothy Nikkel (:tnikkel)]

Frame flag bits seem like they'll last forever now... is this something we really want to spend a flag bit on? I feel a bit guilty using a frame flag bit for this.

Comment 55

[Boris Zbarsky [:bzbarsky]]

It's biting real-world sites....  So I'd go ahead and do it.  If/when we start running out of bits, we can make some hard decisions, but for now this seems like a no-brainer.

Comment 56

[Timothy Nikkel (:tnikkel)]

I didn't know that there was demand for this. So if that's the case then I don't feel so bad using a bit.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Well, at least I ran into a site today that was not showing first-letter styling due to this bug.  ;)

Comment 58

[Gérard Talbot]

Recently added tests in CSS 2.1 test suite based on this bug report:

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/first-line-selector-017.htm

http://test.csswg.org/suites/css2.1/nightly-unstable/html4/first-letter-selector-029.htm

Comment 59

[Phoenix Enero]

Encountered this when I created a "dropcap" button in TinyMCE editor. Please fix (this bug is darn old.)

Comment 60

[Phoenix Enero]

(In reply to Phoenix Enero from comment #59)
> Encountered this when I created a "dropcap" button in TinyMCE editor. Please
> fix (this bug is darn old.)

For those curious, I counteracted this by quickly hiding and showing the element with `display` to force styles refresh (this is achieved by using `setInterval`.)

Comment 62

[Guilherme Lima]

It seems this and bug 1119678 are WFM now.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Pretty sure this is fixed by stylo.

Comment 64

[Emilio Cobos Álvarez (:emilio)]

Indeed! Nice :)