365 bytes, text/html
443 bytes, text/html
11.25 KB, patch
|Details | Diff | Splinter Review|
3.29 KB, text/html; charset=UTF-8
1.92 KB, patch
|Details | Diff | Splinter Review|
320 bytes, text/html
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.
(And I'm not sure whether we have similar, existing, problems with the + selector...)
related to bug 73586
The "+" issue is bug 110972.
milstone bulk change
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 works...
Created attachment 91616 [details] [diff] [review] oops, left off the content/ part of the diff
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...
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.
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.
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.
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.
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...
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. :)
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?
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...
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. ;)
(excluding comment 49)
*** Bug 323052 has been marked as a duplicate of this bug. ***
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.
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.