matching of :empty not dynamically updated [SELECTORS-DYNAMIC]

RESOLVED FIXED in mozilla1.9beta4



CSS Parsing and Computation
16 years ago
8 years ago


(Reporter: sicking, Assigned: dbaron)


(Blocks: 1 bug, {css3, testcase})

css3, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(6 attachments, 1 obsolete attachment)

When you insert a child into an empty element the element will still match 
the :empty selector.

(A cool sideeffect is that it makes it possible for an element to match a 
selector like |p:empty > span| :) )

Will attach a testcase that inserts a textnode in an empty element...
Created attachment 48796 [details]

Comment 2

16 years ago

Comment 3

16 years ago
Can you get this to happen with things other than generated content (i.e.,
properties on the element itself)?
Created attachment 48799 [details]
another testcase
yep, it's not updated at all.. And it works both ways, ie if you remove all 
children from an element it will still not match :empty
Should the CSS frame constructor's ContentInserted() observer be handling this?
It seems that we need to cause some re-resolution of style on content changes...

Comment 7

16 years ago
I think the basic problem is that :empty requires more style re-resolution than
the existing code (which was written before :empty existed) does.  Assigning to
Assignee: dbaron → glazman
Summary: :empty -ness of elements arn't updated when modified by scripts → :empty -ness of elements not dynamically updated

Comment 8

16 years ago
(And I'm not sure whether we have similar, existing, problems with the +
related to bug 73586


16 years ago
Summary: :empty -ness of elements not dynamically updated → matching of :empty not dynamically updated
The "+" issue is bug 110972.
milstone bulk change
Target Milestone: --- → mozilla1.0.1


16 years ago
Summary: matching of :empty not dynamically updated → matching of :empty not dynamically updated [SELECTORS-DYNAMIC]
I have a patch which fixes both of these test cases. I'll atach it so you can
tell me how wrong it is :)
Created attachment 91614 [details] [diff] [review]
v0.1, diff -u

This doesn't touch contentAppended, although I think it probably should. The
logic for the |if(primaryFrame)| stuff in HandleRestyle is ugly too. But it
Created attachment 91616 [details] [diff] [review]
oops, left off the content/ part of the diff
Attachment #91614 - Attachment is obsolete: true

Comment 15

15 years ago
Since mRelevantAttributes is global (rather than per-element), and we have piles
of :empty rules in quirk.css, I don't really see the advantage to adding an
:empty hack to HasRelevantAttributes rather than just doing the appropriate
reresolution every time an element changes from empty to non-empty or
vice-versa.  It would seem like a more important optimization to me to just make
sure that we only do it (i.e., reresolve the parent) when we get an
insertion/removal that changes whether something would match :empty.  I think
that would cause a bit of changes to the refactoring that you'd want to do, so I
haven't reviewed those changes closely...
Hmm. I do only reresolve when the :empty state changes, though. We only have an
:empty rule in quirk.css, though, and the fix for bug 157395 may mean that we
have to remove that rule anyway.

I suspect that this means that we'll (eventually) need a new scheme for handling
dynamic ::nth-child updates dynamically if we don't want to reresolve all
siblings every time a node is moved. How explensive is doing all this restyling,

Whast the difference betwene ContentInserted and ContentAppended? Inserting an
element appends it to a parent node always, doesn't it?
the difference between ContentInserted and ContentAppended is that
ContentAppended is called when the new node is inserted last in it's parents
childlist, i.e. appended to the list of children. ContentInserted is called when
a node is inserted elsewhere in the childlist

Comment 18

15 years ago
I think for things like :nth-child and perhaps even sibling combinators, we can
solve the problem by setting bits on the style context if an element *could have
matched* nth-child, and using those bits to determine whether to reresolve. 
However, I think it's excessive for :empty since I suspect things don't change
whether they match :empty very often.  Furthermore, reresolution for something
that is going in/out of matching :empty will be cheap, since it only has zero or
one children, and a reresolve (what ComputeStyleChangeFor does) always is done
over the entire subtree.  In other words, it seems like premature optimization.

That said, it looks like this patch, in the insert/append case, will be doing
reresolution on the content that was just added, which doesn't really need to
happen unless the rules matched by the parent actually change (and it could be a
significant subtree of content).  That seems like a cost we might not want to
pay (since the reresolve could now be hitting a significant chunk of content,
for example, when building up a document, although we'd probably hit the
perfectly-empty case quite rarely when building up documents).
Ahh, I see. So contentAppended is an optimisation, then? The fact that
ContentAppended takes an arg for the place in teh child list threw me.

I'm getting content inserted calls for the :empty case, though, for both test
cases, even though the second case is calling appendChild - is that expected?
So for the Append/Insert case, I want to reresolve style on the parent before
the child nodes are looked at? How do I do that?

For the more complicated rules, how does :active work? That got fixed not to
require reresolution of eveyrthing, didn't it? (which is why hierarchical hover
is now enabled)

Comment 21

15 years ago
Event state changes use |HasStateDependentStyle|, which is an even stronger
optimization than HasRelevantAttributes (and is also on the correct interface,
nsIStyleRuleProcessor rather than nsIStyleSheet).

However, my comment about the children might be wrong.  We just need to make
sure that the various handlers called by PresShell::ContentInserted /
ContentRemoved / ContentAppended are called in an appropriate order.  (In this
case, I think all the relevant pieces (the reresolution and the frame creation)
are in the frame constructor.)  We don't want to bother resesolving style for
frames we're about to destroy, and likewise we don't want to reresolve style on
the parent before we create the frames for the newly-appended children.  In
other words, where you add the new lines of code matters. :-)
> and likewise we don't want to reresolve style on the parent before we create the
> frames for the newly-appended children.

We don't?  I'd think that we _do_....
> We don't want to bother resesolving style for
> frames we're about to destroy, and likewise we don't want to reresolve style on
> the parent before we create the frames for the newly-appended children.

Hmm. So where am I going wrong in this?

If I have a document <p></p>, and a css rule for p > a, then when I
append(someAElement) we resolve style for the a node, discover it matches that
rule, then walk up the rule tree to fill in the style structs.

If I have another rule for p:empty, then w/o this patch, |p > a|'s parent is the
root node (assuming that there isn't a rule matching the <p>). This is wrong,
which is why dynamic updates dont work.

With my patch, we reresolve the style on the |p|, so that it matches an element
in the rule tree for |p:empty|. In this case, |p > a| has |p:empty|'s rule as a

So don't we _want_ to resolve style on teh parent _before_ appending the child
so that we don't have to change the node's parent in the rule tree, and
reframe/etc the child frames?

<sigh - midair, thats what bz says too. Oh well, at least you can tell me what
else is wrong with my above descirption :)>

Comment 24

15 years ago
Right.  I stuck in a "not" instead of emphasizing the "before".  It made perfect
sense at the time. :-)

Comment 25

15 years ago
(That was a reply to comment 22.  My reply to comment 23 is that it would
probably be correct if you stuck in "'s rule node" and/or "'s rule" in about 5
So, I call DoRestyle in ContentInserted directly after the debug-only printfs at
the top of the function. Is that too late, though? Its before frames are
created, but is it before or after teh style stuff is created for the children?
(ie will I end up creating the children's style tree stuff twice?) I don't think
so, but....

I still want to know why .appendNode calls contentInserted instead of
contentAppended, giving sicking's comment 17.

Re dbaron's comment 25 - yeah, I know, I was just being lazy :)
> is it before or after teh style stuff is created for the children?

Before.  That "style stuff" is created as we create the frames...
==> bbaetz
Assignee: glazman → bbaetz

Comment 29

14 years ago
Depending on what the eventual formal definition of :empty is, we may also need
to be careful about text node data manipulation and replaceChild in addition to
appendChild, insertChild, and removeChild.

Comment 30

14 years ago
Created attachment 127688 [details]
futher testcase

Comment 31

14 years ago
Target Milestone: mozilla1.0.1 → Future
Target Milestone: Future → mozilla1.0.1

Comment 32

14 years ago
Bug 73586, bug 98997, and bug 229915 would be fixed by what I described in step
3 of bug 15608 comment 28:

  When adding/removing content, just reresolve style on the parent (to handle
  empty, +, and all the new CSS3 variants).  (Perhaps there are good ways to
  optimize this, but it might just not be that bad.)

Comment 33

14 years ago
I think what I described in the previous comment is the correct approach for
those three bugs, except for the :last-child half of 73586.  It can be done so
it doesn't affect page load by not modifying ContentAppended.

Comment 34

14 years ago
Er, rather, not modifying ContentAppended except for the case where it goes up
from 0.
Yeah, if we make that check in ContentAppended that seems like a reasonable


14 years ago
Blocks: 75186


14 years ago
Assignee: bbaetz → dbaron
No longer blocks: 75186
Severity: normal → enhancement
Keywords: css3, testcase
OS: other → All
Hardware: Other → All
Summary: matching of :empty not dynamically updated [SELECTORS-DYNAMIC] → matching of :empty not dynamically updated


14 years ago
Blocks: 75186
Please don't remove stuff from the summary for no reason. Also, since we do
support :empty this is a bug and not an enhancement.
Severity: enhancement → normal
Summary: matching of :empty not dynamically updated → matching of :empty not dynamically updated [SELECTORS-DYNAMIC]
Blocks: 188953


12 years ago
Blocks: 296679


12 years ago
Blocks: 305449

Comment 37

12 years ago
I was using Firefox 1.5 beta 1 and created a template that builds a menupopup.
This code works in Firefox 1.0.6 but fails in 1.5 beta 1. I think it is related
to this bug. Can anyone suggest a workaround?
	   <rule rdf:type="">

	      <menu class="menu-iconic bookmark-item" uri="rdf:*"






           <rule parent="menu">


            <menuitem class="menuitem-iconic bookmark-item" uri="rdf:*"








Comment 38

12 years ago
Asking for workarounds, especially with huge examples like that, doesn't belong
in bugs.

Comment 39

12 years ago
(In reply to comment #38)
> Asking for workarounds, especially with huge examples like that, doesn't belong
> in bugs.

The sample code was just to show how this bug will effect extensions being moved
from Firefox 1.0 to 1.5. Adding nodes where the element is empty will cause
templates to fail breaking a lot of extensions that use templates to build
menus. I would think that would be considered a major problem with layout.


12 years ago
Blocks: 307045

Comment 40

12 years ago
So, I have a patch that fixed both testcases in this bug. Do we have any other testcases for :empty that should be tested? (I need to find a good testcase of the menu:empty issue from xul.css)

Comment 41

12 years ago
Ok, seems to not help the testcase in bug 307045. I wonder why...

Comment 42

12 years ago
Ok, bug 307045 is not caused by this bug - the :empty selector is working fine there (it's screwing over the template builder, but that's another story).

So the patch I've got fixes this bug, bug 73586 (at least the two testcases work), and bug 229915. Woo. The patch more or less just does exactly what David Baron said in bug 73586 comment 11 anyway, so I guess it's not so surprising it works. :) There's no optimisation, mind, so I wouldn't like to bet on the PageLoad effect of it...

Comment 43

12 years ago
Created attachment 201940 [details] [diff] [review]
Reference patch

This is the patch I currently have that fixed all the testcases mentioned previously. Need to work out how to run the pageload tests to see just how badly it performs there. :)

Comment 44

12 years ago
It's worth considering not only the effect on "normal" pages but also the effect on long, linear pages, on which O(N^2) algorithms show up the worst.  I did a good bit of work a few years back to make a page with 100000 "<p>This is a paragraph.</p>", each on its own line, load faster:  bug 86947 (see bug 86947 comment 43), bug 61962, bug 111676, and I think another one that I'm missing.  I suspect it's regressed significantly since then due to some of roc's recent nsBlockFrame changes, but this would probably make it worse.

Comment 45

12 years ago
Yeah, just doing the restyles blindly as I've currently got it would be absolutely hidious on any sort of big page. :)

I guess the question now is how to optimise it. I read bug 73586 comment 12, but I don't (yet) know what would be done with the extra hashtable. Also, would using a global state for the nth-* (and related) stuff mean that a page would simply lose some/most/all of the optimisations the moment it used one of those selectors?
Blocks: 315264

Comment 46

12 years ago
So I'm thinking that a hashtable would be overkill, and we could get away with using a few bits on style context:
 + has :empty-related selectors,
 + is empty,
 + has child that could match :first-child,
 + has child that could match :nth-* variants that count from start,
 + has child that could match :last-child, and
 + has child that could match :nth-* variants that count from end.
The interaction of implementing the last 4 of these with the event state and attribute optimizations in SelectorMatches is quite tricky, though.  i.e., it's one of those things I'd rather do myself.

(Now that I think about it, there are bugs in our handling of event state and attribute changes on the same element.)

Comment 47

12 years ago
(In reply to comment #46)
> (Now that I think about it, there are bugs in our handling of event state and
> attribute changes on the same element.)

Actually, the current code is fine, since the whether-we-need-to-reresolve tests are always based on current information rather than old information, as would happen in my proposal.
Actually, I'm not sure about that. I think a selector like:


wouldn't do the right thing if that attribute is removed or added...

Comment 49

12 years ago
Er, never mind the suggestion of bits:  it doesn't handle combinators, at least not without the out-of-band hashtable.

And that selector would do the right thing (it's pretty much the case I was thinking of, although I was using onmouseover/onmouseout to set attributes and using an attribute selector plus :hover), because whichever change is processed second will trigger the current code, since (as I said in comment 47), the test is based on current information, so at the second change we will see that something needs to happen.
Ah, good point.  Assuming we update both sources of information before sending either notification (which I think we do).

Comment 51

12 years ago
(In reply to comment #50)
> Ah, good point.  Assuming we update both sources of information before sending
> either notification (which I think we do).

Actually, it only works if we *don't* do that, but instead if we update the information and then send the notification.  (In that case it would happen to work going into the states, but not coming out; for :not() around both, the reverse would be true.)

Comment 52

12 years ago
... by which I mean update and notify sequentially for each piece of information to be updated.
For the [selected]:checked case, the underlying information for both parts is exactly the same -- whether the attribute is set...
Blocks: 315296
I thought the selected="" attribute set the defaultSelected state, and that the :checked pseudo-class matches on the current selection state? i.e. that they were independent. But I haven't checked.
Er, indeed.  Bad example.  Try "[disabled]:disabled" instead.  ;)

Comment 56

12 years ago
Issue in comment 47 through comment 55 filed as bug 315920.

Comment 57

12 years ago
(excluding comment 49)

Comment 58

12 years ago
*** Bug 323052 has been marked as a duplicate of this bug. ***


12 years ago
No longer blocks: 188953

Comment 59

12 years ago
Created attachment 208256 [details]
Test of non empty BODY element

Test case to show that a non dynamicaly filled BODY element still registers as empty.


12 years ago
Blocks: 323052


10 years ago
Assignee: dbaron → nobody
QA Contact: ian → style-system


10 years ago
Depends on: 401291
Blocks: 410460


10 years ago
Blocks: 404418

Comment 60

10 years ago
Fix by checkin of bug 401291 to trunk, 2008-02-18 22:17 -0800.

I included tests for this bug (based on attachment 127688 [details]) in that landing.
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.0.1 → mozilla1.9beta4


10 years ago
Assignee: nobody → dbaron


10 years ago
Blocks: 372632
You need to log in before you can comment on or make changes to this bug.