Last Comment Bug 8253 - Can't dynamically add ::first-line / ::first-letter style
: Can't dynamically add ::first-line / ::first-letter style
Status: NEW
[Hixie-P2][CSS1-2.3][CSS1-2.4] dynami...
: css1, dom2, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal with 11 votes (vote)
: Future
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
: 82289 193253 231205 347296 414762 470771 (view as bug list)
Depends on: 144826
Blocks: 145419 303076 559612 css2.1-tests 23701 320451 375758
  Show dependency treegraph
 
Reported: 1999-06-15 16:41 PDT by Peter Linss
Modified: 2016-05-16 02:24 PDT (History)
31 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (as attachment for convenience) (306 bytes, text/html)
1999-07-11 04:51 PDT, Christian Wenz
no flags Details
Testcase for dynamc ":before" (82 bytes, text/html)
1999-10-08 18:04 PDT, Peter Linss
no flags Details
Inclusion of :first-line oddity. (541 bytes, text/html)
2002-02-20 00:05 PST, Mike McCaughan
no flags Details
Testcase that shows the first-line problem more clearly (401 bytes, text/html)
2004-05-29 11:42 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testcase that shows the first-letter problem more clearly (5.63 KB, patch)
2004-05-29 11:47 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Real first-letter testcase (366 bytes, text/html)
2004-05-29 13:16 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testcase showing that changing the class of an element applies new CSS rules excpet for :first-line (436 bytes, text/html)
2006-05-13 13:58 PDT, Giuseppe Bilotta
no flags Details
patch (6.82 KB, patch)
2009-11-13 00:31 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch v2 (12.14 KB, patch)
2009-11-15 22:20 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch v3 (22.34 KB, patch)
2009-11-19 17:37 PST, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 1. Disable first-letter and first-line styles in select elements. (994 bytes, patch)
2010-05-15 01:18 PDT, Timothy Nikkel (:tnikkel)
bzbarsky: review+
Details | Diff | Splinter Review
Part 2. Allow first-letter and first-line styles to be applied dynamically. (15.23 KB, patch)
2010-05-15 01:19 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 3. Optimize checking for existing first-line style in frame constructor. (10.11 KB, patch)
2010-05-15 01:20 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review

Description Peter Linss 1999-06-15 16:41:45 PDT
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 1999-07-06 15:32:59 PDT
Changing to M9.
Comment 2 Christian Wenz 1999-07-11 04:51:59 PDT
Created attachment 808 [details]
test case (as attachment for convenience)
Comment 3 Christian Wenz 1999-07-11 04:57:59 PDT
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 peterl@netscape.com's
comment.
Changing bug status to "[TESTCASE] ..." standard.
Comment 4 kipp 1999-07-14 10:54:59 PDT
Dynamic addition of first-line style now works; there is zero support for
dynamic first-letter (see bug #6059).
Comment 5 kipp 1999-08-19 08:22:59 PDT
Since you are working on moving ReResolveStyle into the frame construction
code...
Comment 6 Peter Linss 1999-10-08 18:02:59 PDT
Dynamic addition of ":before" and ":after" pseudo elements doesn't work either.
Comment 7 Peter Linss 1999-10-08 18:04:59 PDT
Created attachment 2043 [details]
Testcase for dynamc ":before"
Comment 8 Pierre Saslawsky 1999-10-21 01:07:59 PDT
Reassigning peterl's bugs to myself.
Comment 9 Pierre Saslawsky 1999-10-21 01:12:59 PDT
Accepting peterl's bugs that have a Target Milestone
Comment 10 Pierre Saslawsky 1999-12-20 21:11:59 PST
Pushing my M15 bugs to M16
Comment 11 ekrock's old account (dead) 2000-01-21 13:52:21 PST
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Comment 12 Pierre Saslawsky 2000-05-07 20:05:23 PDT
Latered.
Comment 13 Hixie (not reading bugmail) 2001-02-12 16:32:35 PST
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) 2001-06-13 22:19:02 PDT
Reopening and moving to Future...
Comment 15 Mike McCaughan 2002-02-20 00:05:37 PST
Created attachment 70480 [details]
Inclusion of :first-line oddity.

Adding feul to the fire...
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2002-02-26 18:18:21 PST
This is not a problem for stylesheet switching too, not just dynamic stuff,
since switching just reresolves style....
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2002-02-26 18:41:22 PST
er, "This is now a problem"

Comment 18 David Baron :dbaron: ⌚️UTC-10 2002-06-19 21:07:54 PDT
Assigning pierre's remaining Style System-related bugs to myself.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2003-02-13 22:02:59 PST
*** Bug 82289 has been marked as a duplicate of this bug. ***
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2003-02-13 22:03:15 PST
*** Bug 193253 has been marked as a duplicate of this bug. ***
Comment 21 David Baron :dbaron: ⌚️UTC-10 2004-01-16 17:51:39 PST
*** Bug 231205 has been marked as a duplicate of this bug. ***
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2004-05-29 11:42:28 PDT
Created attachment 149591 [details]
Testcase that shows the first-line problem more clearly
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2004-05-29 11:47:10 PDT
Created attachment 149592 [details] [diff] [review]
Testcase that shows the first-letter problem more clearly
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2004-05-29 13:16:07 PDT
Created attachment 149597 [details]
Real first-letter testcase
Comment 25 Giuseppe Bilotta 2006-05-13 13:54:29 PDT
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 2006-05-13 13:58:52 PDT
Created attachment 221927 [details]
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) 2006-08-05 19:17:17 PDT
*** Bug 347296 has been marked as a duplicate of this bug. ***
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2007-04-16 01:15:57 PDT
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-10 2007-05-12 13:41:24 PDT
See also bug 379799.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2008-02-04 19:59:09 PST
*** Bug 414762 has been marked as a duplicate of this bug. ***
Comment 31 Daniel.S 2008-12-22 08:32:52 PST
*** Bug 470771 has been marked as a duplicate of this bug. ***
Comment 32 Timothy Nikkel (:tnikkel) 2009-11-13 00:31:53 PST
Created attachment 412154 [details] [diff] [review]
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 [:bz] (still a bit busy) 2009-11-13 21:34:08 PST
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) 2009-11-15 22:17:02 PST
(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) 2009-11-15 22:20:06 PST
Created attachment 412521 [details] [diff] [review]
patch v2

Updated based on comments.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 06:23:05 PST
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 [:bz] (still a bit busy) 2009-11-16 09:42:09 PST
> 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) 2009-11-19 17:37:19 PST
Created attachment 413487 [details] [diff] [review]
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, jst@mozilla.com) 2010-05-10 18:07:56 PDT
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 [:bz] (still a bit busy) 2010-05-10 18:46:48 PDT
I would actually really like to at least glance at this before checkin, once roc's ok with it.
Comment 41 Timothy Nikkel (:tnikkel) 2010-05-10 18:48:18 PDT
There's absolutely no hurry on this bug...
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-10 19:45:11 PDT
+    // 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) 2010-05-15 01:18:34 PDT
Created attachment 445527 [details] [diff] [review]
Part 1. Disable first-letter and first-line styles in select elements.
Comment 44 Timothy Nikkel (:tnikkel) 2010-05-15 01:19:41 PDT
Created attachment 445528 [details] [diff] [review]
Part 2. Allow first-letter and first-line styles to be applied dynamically.
Comment 45 Timothy Nikkel (:tnikkel) 2010-05-15 01:20:24 PDT
Created attachment 445529 [details] [diff] [review]
Part 3. Optimize checking for existing first-line style in frame constructor.
Comment 46 Timothy Nikkel (:tnikkel) 2010-05-15 01:23:12 PDT
Ready for review, not sure who wants to review what though.
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2010-05-17 08:27:45 PDT
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 [:bz] (still a bit busy) 2010-05-17 08:39:23 PDT
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 [:bz] (still a bit busy) 2010-05-17 08:42:19 PDT
Haven't looked at part 3 yet, pending us sorting out part 2.
Comment 50 Boris Zbarsky [:bz] (still a bit busy) 2010-05-17 08:42:39 PDT
And in particular, I'd want to see some perf data on part 3.
Comment 51 Timothy Nikkel (:tnikkel) 2010-06-05 16:25:19 PDT
Pushed part 1
http://hg.mozilla.org/mozilla-central/rev/b2d0bd7761a1
Comment 52 Timothy Nikkel (:tnikkel) 2010-06-08 20:10:53 PDT
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 [:bz] (still a bit busy) 2011-06-28 23:05:19 PDT
Timothy, you want to pick this up again now that we have free flag bits?
Comment 54 Timothy Nikkel (:tnikkel) 2011-06-28 23:11:59 PDT
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 [:bz] (still a bit busy) 2011-06-28 23:15:10 PDT
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) 2011-06-28 23:17:21 PDT
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 [:bz] (still a bit busy) 2011-06-28 23:26:29 PDT
Well, at least I ran into a site today that was not showing first-letter styling due to this bug.  ;)
Comment 59 Phoenix Enero 2016-04-02 04:26:14 PDT
Encountered this when I created a "dropcap" button in TinyMCE editor. Please fix (this bug is darn old.)
Comment 60 Phoenix Enero 2016-04-02 04:41:41 PDT
(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`.)

Note You need to log in before you can comment on or make changes to this bug.