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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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...

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase...

Comment 2

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

weird.

Comment 3

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

Can you get this to happen with things other than generated content (i.e.,
properties on the element itself)?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: another testcase

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

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

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
Daniel.

Comment 8

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

(And I'm not sure whether we have similar, existing, problems with the +
selector...)

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

related to bug 73586

Comment 10

[Hixie (not reading bugmail)]

The "+" issue is bug 110972.

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

milstone bulk change

Comment 12

[Bradley Baetz (:bbaetz)]

I have a patch which fixes both of these test cases. I'll atach it so you can
tell me how wrong it is :)

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: 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
works...

Comment 14

[Bradley Baetz (:bbaetz)]

Attachment: oops, left off the content/ part of the diff

Comment 15

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

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...

Comment 16

[Bradley Baetz (:bbaetz)]

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,
anyway?

Whast the difference betwene ContentInserted and ContentAppended? Inserting an
element appends it to a parent node always, doesn't it?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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

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).

Comment 19

[Bradley Baetz (:bbaetz)]

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?

Comment 20

[Bradley Baetz (:bbaetz)]

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

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

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. :-)

Comment 22

[Boris Zbarsky [:bzbarsky]]

> 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_....

Comment 23

[Bradley Baetz (:bbaetz)]

> 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
parent.

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

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

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

Comment 25

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

(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
places.)

Comment 26

[Bradley Baetz (:bbaetz)]

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 :)

Comment 27

[Boris Zbarsky [:bzbarsky]]

> is it before or after teh style stuff is created for the children?

Before.  That "style stuff" is created as we create the frames...

Comment 28

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

==> bbaetz

Comment 29

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

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

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

Attachment: futher testcase

Comment 31

[Vedran Miletic]

retargeting

Comment 32

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

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

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

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

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

Er, rather, not modifying ContentAppended except for the case where it goes up
from 0.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Yeah, if we make that check in ContentAppended that seems like a reasonable
approach.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 37

[mark bokil]

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="http://home.netscape.com/NC-rdf#Livemark">

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

              label="rdf:http://home.netscape.com/NC-rdf#Name"

              rdf:type="http://home.netscape.com/NC-rdf#Folder"

              livemark="true">

           </menu>

	   </rule

           <rule parent="menu">

            <menupopup>

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

                  label="rdf:http://home.netscape.com/NC-rdf#Name" 

                  image="rdf:http://home.netscape.com/NC-rdf#Icon"

                  status="rdf:http://home.netscape.com/WEB-rdf#status"

                  statustext="rdf:http://home.netscape.com/NC-rdf#URL"

                  metadata="rdf:http://home.netscape.com/NC-rdf#WebPanel"/>

            </menupopup>

        </rule>

Comment 38

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

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

Comment 39

[mark bokil]

(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.

Comment 40

[James Ross]

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

[James Ross]

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

Comment 42

[James Ross]

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

[James Ross]

Attachment: 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

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

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

[James Ross]

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?

Comment 46

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

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

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

(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.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Actually, I'm not sure about that. I think a selector like:

  option[selected]:checked

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

Comment 49

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

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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Ah, good point.  Assuming we update both sources of information before sending either notification (which I think we do).

Comment 51

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

(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

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

... by which I mean update and notify sequentially for each piece of information to be updated.

Comment 53

[Boris Zbarsky [:bzbarsky]]

For the [selected]:checked case, the underlying information for both parts is exactly the same -- whether the attribute is set...

Comment 54

[Hixie (not reading bugmail)]

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.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Er, indeed.  Bad example.  Try "[disabled]:disabled" instead.  ;)

Comment 56

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

Issue in comment 47 through comment 55 filed as bug 315920.

Comment 57

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

(excluding comment 49)

Comment 58

[Adam Guthrie]

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

Comment 59

[Simon Proctor]

Attachment: Test of non empty BODY element

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

Comment 60

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

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.