Closed Bug 98997 Opened 23 years ago Closed 17 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta4

People

(Reporter: sicking, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: css3, testcase)

Attachments

(6 files, 1 obsolete file)

testcase...
365 bytes, text/html
Details
another testcase
443 bytes, text/html
Details
oops, left off the content/ part of the diff
11.25 KB, patch
Details | Diff | Splinter Review
futher testcase
3.29 KB, text/html; charset=UTF-8
Details
Reference patch
1.92 KB, patch
Details | Diff | Splinter Review
Test of non empty BODY element
320 bytes, text/html
Details 
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...
Can you get this to happen with things other than generated content (i.e.,
properties on the element itself)?
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...
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.
Assignee: dbaron → glazman
Summary: :empty -ness of elements arn't updated when modified by scripts → :empty -ness of elements not dynamically updated
(And I'm not sure whether we have similar, existing, problems with the +
selector...)
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
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 :)
Attached patch v0.1, diff -u (obsolete) — — Splinter Review 
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...
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,
anyway?

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
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)
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
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 :)>
Right.  I stuck in a "not" instead of emphasizing the "before".  It made perfect
sense at the time. :-)
(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.)
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
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.
retargeting
Target Milestone: mozilla1.0.1 → Future
Target Milestone: Future → mozilla1.0.1
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.)
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.
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
approach.
Blocks: 75186
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
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
Blocks: 296679
Blocks: 305449
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>
Asking for workarounds, especially with huge examples like that, doesn't belong
in bugs.
(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.
Blocks: 307045
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)
Ok, seems to not help the testcase in bug 307045. I wonder why...
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...
Attached patch Reference patch — — Splinter Review 
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. :)
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.
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
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.)
(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:

  option[selected]:checked

wouldn't do the right thing if that attribute is removed or added...
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).
(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.)
... 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.  ;)
*** Bug 323052 has been marked as a duplicate of this bug. ***
Test case to show that a non dynamicaly filled BODY element still registers as empty.
Assignee: dbaron → nobody
QA Contact: ian → style-system
Blocks: acid3
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.
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.0.1 → mozilla1.9beta4
Assignee: nobody → dbaron
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: