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

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P3
normal
18 years ago
5 months ago

People

(Reporter: Peter Linss, Assigned: tnikkel)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs, {css1, dom2, testcase})

Trunk
Future
css1, dom2, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P2][CSS1-2.3][CSS1-2.4] dynamically adding styles not implemented)

Attachments

(6 attachments, 7 obsolete attachments)

401 bytes, text/html
Details
366 bytes, text/html
Details
436 bytes, text/html
Details
994 bytes, patch
Details | Diff | Splinter Review
15.23 KB, patch
Details | Diff | Splinter Review
10.11 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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.
Summary: Can't dynamically add :first-line / :first-letter style → {first-letter} Can't dynamically add :first-line / :first-letter style
Target Milestone: M15

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: M15 → M8

Updated

18 years ago
Target Milestone: M8 → M9

Comment 1

18 years ago
Changing to M9.

Comment 2

18 years ago
Created attachment 808 [details]
test case (as attachment for convenience)

Updated

18 years ago
Whiteboard: [TESTCASE] dynamically adding styles not implemented

Comment 3

18 years ago
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.

Updated

18 years ago
QA Contact: petersen → chrisd

Updated

18 years ago
Priority: P2 → P3
Target Milestone: M9 → M15

Comment 4

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

Updated

18 years ago
Assignee: kipp → peterl
Status: ASSIGNED → NEW

Comment 5

18 years ago
Since you are working on moving ReResolveStyle into the frame construction
code...
(Reporter)

Comment 6

18 years ago
Dynamic addition of ":before" and ":after" pseudo elements doesn't work either.
(Reporter)

Comment 7

18 years ago
Created attachment 2043 [details]
Testcase for dynamc ":before"

Comment 8

18 years ago
Reassigning peterl's bugs to myself.

Comment 9

18 years ago
Accepting peterl's bugs that have a Target Milestone

Comment 10

18 years ago
Pushing my M15 bugs to M16
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Keywords: testcase

Comment 12

18 years ago
Latered.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → LATER
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...
QA Contact: chrisd → ian
Reopening and moving to Future...
Status: RESOLVED → REOPENED
Keywords: css1, dom2
Resolution: LATER → ---
Whiteboard: [TESTCASE] dynamically adding styles not implemented → [Hixie-P2] dynamically adding styles not implemented
Target Milestone: M16 → Future

Comment 15

16 years ago
Created attachment 70480 [details]
Inclusion of :first-line oddity.

Adding feul to the fire...
This is not a problem for stylesheet switching too, not just dynamic stuff,
since switching just reresolves style....
OS: Windows NT → All
Hardware: PC → All
er, "This is now a problem"

Whiteboard: [Hixie-P2] dynamically adding styles not implemented → [Hixie-P2][CSS1-2.3][CSS1-2.4] dynamically adding styles not implemented
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: REOPENED → NEW
*** Bug 82289 has been marked as a duplicate of this bug. ***
*** Bug 193253 has been marked as a duplicate of this bug. ***
Blocks: 145419
Component: Layout → Style System
Depends on: 144826
*** Bug 231205 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Summary: {first-letter} Can't dynamically add :first-line / :first-letter style → Can't dynamically add ::first-line / ::first-letter style
Created attachment 149591 [details]
Testcase that shows the first-line problem more clearly
Attachment #808 - Attachment is obsolete: true
Attachment #2043 - Attachment is obsolete: true
Attachment #70480 - Attachment is obsolete: true
Created attachment 149592 [details] [diff] [review]
Testcase that shows the first-letter problem more clearly
Created attachment 149597 [details]
Real first-letter testcase
Attachment #149592 - Attachment is obsolete: true

Updated

13 years ago
Blocks: 23701

Updated

12 years ago
Blocks: 320451

Comment 25

11 years ago
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

11 years ago
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.
*** Bug 347296 has been marked as a duplicate of this bug. ***
Blocks: 375758
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.  :(
See also bug 379799.
Assignee: dbaron → nobody
QA Contact: ian → style-system
Duplicate of this bug: 414762
Blocks: 303076

Updated

9 years ago
Duplicate of this bug: 470771
(Assignee)

Comment 32

8 years ago
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.
Assignee: nobody → tnikkel
Attachment #412154 - Flags: review?(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?
(Assignee)

Comment 34

8 years ago
(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.
(Assignee)

Comment 35

8 years ago
Created attachment 412521 [details] [diff] [review]
patch v2

Updated based on comments.
Attachment #412521 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #412154 - Attachment is obsolete: true
Attachment #412154 - Flags: review?(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.
> 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?
(Assignee)

Comment 38

8 years ago
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.
Attachment #412521 - Attachment is obsolete: true
Attachment #412521 - Flags: review?(bzbarsky)
Attachment #413487 - Flags: review?(bzbarsky)

Updated

7 years ago
Blocks: 559612
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.
Attachment #413487 - Flags: review?(bzbarsky) → review?(roc)
I would actually really like to at least glance at this before checkin, once roc's ok with it.
(Assignee)

Comment 41

7 years ago
There's absolutely no hurry on this bug...
Attachment #413487 - Flags: review?(bzbarsky)
+    // 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?
(Assignee)

Comment 43

7 years ago
Created attachment 445527 [details] [diff] [review]
Part 1. Disable first-letter and first-line styles in select elements.
(Assignee)

Updated

7 years ago
Attachment #413487 - Attachment is obsolete: true
Attachment #413487 - Flags: review?(roc)
Attachment #413487 - Flags: review?(bzbarsky)
(Assignee)

Comment 44

7 years ago
Created attachment 445528 [details] [diff] [review]
Part 2. Allow first-letter and first-line styles to be applied dynamically.
(Assignee)

Comment 45

7 years ago
Created attachment 445529 [details] [diff] [review]
Part 3. Optimize checking for existing first-line style in frame constructor.
(Assignee)

Comment 46

7 years ago
Ready for review, not sure who wants to review what though.
Comment on attachment 445527 [details] [diff] [review]
Part 1. Disable first-letter and first-line styles in select elements.

r=bzbarsky
Attachment #445527 - Flags: review+
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?
Haven't looked at part 3 yet, pending us sorting out part 2.
And in particular, I'd want to see some perf data on part 3.
(Assignee)

Comment 51

7 years ago
Pushed part 1
http://hg.mozilla.org/mozilla-central/rev/b2d0bd7761a1
(Assignee)

Comment 52

7 years ago
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.
Blocks: 605520
Timothy, you want to pick this up again now that we have free flag bits?
(Assignee)

Comment 54

6 years ago
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.
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.
(Assignee)

Comment 56

6 years ago
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.
Well, at least I ran into a site today that was not showing first-letter styling due to this bug.  ;)

Comment 58

5 years ago
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
See Also: → bug 1119678

Comment 59

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

Comment 60

a year ago
(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`.)
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1344547
You need to log in before you can comment on or make changes to this bug.