From bug 53974: Text nodes are being passed into SelectorMatches. It is unclear if this should be prevented entirely, or if SelectorMatches needs to be smarter about how it handles text nodes. Some research needs to be done on this, and there is some good commentary in bug 53974. Some of the proposed new selectors for CSS3 may make it a requirement that the text nodes go into SelectorMatches (I'm thinking of the proposed :contains pseudo element, specifically). Currently, there is no need since no supported selectors match text nodes.

Accepting, moving to Future to investigate post-RTM.

Marc, Pierre: Could this help performance? Nominating nsbeta1 so that we at least consider this.

I think we can actually prevent all rule matching on text nodes, which is a tad better than just short-circuiting SelectorMatches. My only concern is what happens later, when CSS3 selectors get implemented, so I'll comment the hell out of the change when I make it (assuming it provides significant benefit).

CSS3 won't match text nodes, ever, either. (:contains is a pseudo-class and matches elements not text nodes). Maybe far in the future text nodes might be matchable, but even then, I'd guess matching on a *part* of a text node would be much more likely to be what ends up being allowed.

I've attached a path that prevents text nodes from getting into rule matching, and asserts in SelectorMatches to make sure I caught them all. I tested this for a potential performance improvement, using a large text document (tinderbox full build log) and could not see more than a 5% performance improvement in style resolution, which could be just noise (used the Gecko Performance Test loading the log 10 times).

Patch is attached, needs review...

r=pierre

I suspect more performance win could be seen on pages that actually have many textnodes, e.g., pages with plenty <tag>'s and short texts in between.

Added testcase to kwds - it would be nice to have a testcase that showed some performance advantage (rbs?). I think that this should be landed anyway, hence the request for reviews, since it is also a correctness issue and it has not introduced any performance degradation in the common cases.

Didn't we decide that doing QI() on XUL elements is fairly expensive? Do we know why we end up trying to match selectors on text nodes in the first place?

Yes, QI on an XUL element (for an interface that it does not support) is expensive. I could put in the hack I added before to pre-screen XUL elements and assume that a XULElement cannot be a text node - assuming that XUL elements are never text nodes too seems reasonable (CC'ing Hyatt to make sure). The reason we are trying to match selectors against text nodes, and the reason for this bug, is because ResolveStyleContextFor can be called by anyone, and they can pass in text nodes for aContent - in fact they do. Alternately we could make all 20 or so existing callers of ResolveStyleContextFor screen out text nodes but I thought that it was safest to do it in the rule matching code. Alternatively we could screen textnodes in nsPresContext::ResolveStyleContextFor and maybe even return an error from there... hmm, I hadn't thought of that. That would provide the benefit of publicizing the semantic that ResolveStyleContextFor is not to be called for text nodes, allowing clients to do their own optimizations, potentially. Thoughts on that?

Here is another way we end up trying to match selectors on text nodes: calls to nsCSSFrameConstructor::ContentStatesChanged end up calling nsStyleSetImpl::HasStateDependentStyle which does selector matching.

Gotcha. sr=waterson

Milestone shift --> Moz 0.8.1

looks good. r/sr=waterson

fine with me too, r=pierre

Fix chececked in: nsCSSStyleSheet.cpp

The fix caused regressions - bug 71561 at least. It seems that text elements are being used by comboboxes, and that style is being resolved on them using the :-moz-display-comboboxcontrol-frame pseudo, so we really do have to allow them into selector matches (at least those particular text elements). I'll need a better way to screen out the text nodes that really should not be getting styled at all, ever. Backing out the change, so reopening this bug.

There is at least one case (comboboxes) where text nodes need to be styled (and hence need to be getting into SelectorMatches). Of course, we could decide that comboboxes are wrong and fix them, or we could find a more targetted approach to identifying the text nodes that we actually want to keep out of SelectorMatches. Moving to Future for now since this is not critical and had very little if any performance benefits anyway.

The way comboboxes are styling text nodes seems wrong -- I wonder if they could do what that does by styling the OPTION element. It seems to me like a better long-term fix would be to fix comboboxes not to require styling of text nodes (i.e., not to depend on a style system bug) and then re-enable this fix.

The combo box problem will be fixed when ("when") we switch to XBL widgets. In the meantime, shouldn't the combo box be using a pseudo-*element*, instead?

David, Ian - thanks for confirming my feeling about how the combobox is using the text node (styled no less!). I'll get Rod involved in this conversation and see what his feelings are. David, you said it most plainly: by styling the text element, comboboxes are taking advantage of a bug in the style system that we want to fix. CC'ing rod (and emailing him some background data).

The combobox is NOT styling the text nodes in the options. It's styling the text node in the display area. In fact, it used the same style on the containing block and on the text node: // create the style context for the anonymous frame nsCOMPtr<nsIStyleContext> styleContext; rv = aPresContext->ResolvePseudoStyleContextFor(mContent, nsHTMLAtoms::mozDisplayComboboxControlFrame, mStyleContext, PR_FALSE, getter_AddRefs(styleContext)); if (NS_FAILED(rv)) { return rv; } if (!styleContext) { return NS_ERROR_NULL_POINTER; } // create a text frame and put it inside the block frame rv = NS_NewTextFrame(shell, &mTextFrame); if (NS_FAILED(rv)) { return rv; } if (!mTextFrame) { return NS_ERROR_NULL_POINTER; } nsCOMPtr<nsIStyleContext> textStyleContext; rv = aPresContext->ResolvePseudoStyleContextFor(mContent, nsHTMLAtoms::mozDisplayComboboxControlFrame, styleContext, PR_FALSE, getter_AddRefs(textStyleContext)); My guess is the text node is being styled because for some reason styling the parent block frame didn't do what it was suppose do. And since layout is so easy to understand, I am sure it was just laziness I didn't figure why :) So the point is, if we could figure out why the block isn't getting the right style applied, or we could at least understand why it isn't doing what I think it ought to be doing, we could take the style off the TextNode.

Rod, it looks like that code was added over a year ago, so maybe we should revisit it and see if it is still necessary. I'll open a new bug on it to keep this one from getting too off-track.

Opened bug 71717 for the ComboboxControlFrame issue - marking dependent.

Matching milestone to the bug this depends on.

See related bug 74598 describing a possible further improvement to text node style context management.

Patch is attached, but waiting for dependency to be fixed.

Matching milestone to dependent bug (it changed today to 0.9.1)

If we can get this fixed, I'd love to then look into the issue of eliminating style contexts from text frames all together (and just letting them use their parent's style context to draw). This would allow us to gain back some important time during reflow in addition to style resolution, since individual text frames obtain fonts (and call the ever-expensive GetMetricsFor).

Moving P2 and P3 bugs to 0.9.2

By mandate, moving non-crashers and non-dataloss bugs out.

Taking -- I want to try to get this in and fix the dependency (and then work on the issue hyatt mentioned).

I also attached a patch to bug 71717 that will allow this to be checked in.

Comment on attachment 55328 [details] [diff] [review] updated patch (also includes changes for bug 98128) r=pierre

sr=hyatt

OK, I started digging a bit more, and I found some interesting things: * most text nodes have their style contexts created using a pseudo-element (textNodePseudo), which is cheaper than creating them using the content node itself, since we don't have to match against universal rules. Apparently some comments and processing instructions can have style resolved on them as well, through pseudos as well (eek!). * there were a few that were actually passing the text node, and I fixed them not to pass the text node but to use the textPseudo like everwhere else. I'll attach a more compelete patch that turns the IsContentOfType checks in the style set / rule processor code into assertions and fixes up the callers. There's still a good bit more work we can do here...

(FWIW, the only time selectors *matched* text nodes was the case described in bug 71717.)

In the above patch I omitted the following change: Index: layout/html/document/src/forms.css =================================================================== RCS file: /cvsroot/mozilla/layout/html/document/src/forms.css,v retrieving revision 3.27 diff -u -d -r3.27 forms.css --- forms.css 2001/10/22 10:39:35 3.27 +++ forms.css 2001/10/27 04:56:27 @@ -217,7 +217,7 @@ textarea[disabled], option[disabled], select[disabled], -select[disabled] > :-moz-display-comboboxcontrol-frame { +select[disabled]:-moz-display-comboboxcontrol-frame { color: GrayText; cursor: default; }

Comment on attachment 55350 [details] [diff] [review] more complete patch - prevent text nodes from being passed to style system sr=attinasi

Sorry, before spending more time looking at this one, I'm actually working on a better one that stops us doing any style resolution at all for non-elements (although we still create style contexts). I'll attach it shortly.

Comment on attachment 55604 [details] [diff] [review] more complete patch -- prevent any style resolution on non-elements, including using pseudos sr=attinasi

Comment on attachment 55604 [details] [diff] [review] more complete patch -- prevent any style resolution on non-elements, including using pseudos r=hyatt

Put // XXX indicating that the hack at the top of GetFirstLetterStyle in nsBlockFrame is temporary. Do the same with the ResolveStyleForNonElement declaration in the interface.

Checked in 2001-10-29 22:02/36 PDT.