Closed Bug 56117 Opened 24 years ago Closed 23 years ago

[Patch-Dep] Text Nodes should not be matching any selectors (keep out of SelectorMatches?)

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: attinasi, Assigned: dbaron)

References

Details

(Keywords: perf, testcase)

Attachments

(5 files)

Patch to keep text nodes out of rule-matching
2.96 KB, patch
Details | Diff | Splinter Review
Patch that addresses the QI-on-XUL issue
3.72 KB, patch
Details | Diff | Splinter Review
updated patch (also includes changes for bug 98128)
8.64 KB, patch
pierre
: review+
Details | Diff | Splinter Review
more complete patch - prevent text nodes from being passed to style system
20.39 KB, patch
attinasi
: superreview+
Details | Diff | Splinter Review
more complete patch -- prevent any style resolution on non-elements, including using pseudos
50.45 KB, patch
hyatt
: review+
attinasi
: superreview+
Details | Diff | Splinter Review 
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.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Marc, Pierre: Could this help performance?

Nominating nsbeta1 so that we at least consider this.
Keywords: nsbeta1, perf
QA Contact: chrisd → ian
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).
Target Milestone: Future → mozilla0.9
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...
Keywords: patch
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.
Keywords: testcase
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
Target Milestone: mozilla0.9 → mozilla0.8.1
looks good. r/sr=waterson
fine with me too, r=pierre
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.8.1 → Future
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.
Depends on: 71717
Matching milestone to the bug this depends on.
Target Milestone: Future → mozilla0.9
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.
Summary: Text Nodes should not be matching any selectors (keep out of SelectorMatches?) → [Patch-Dep] Text Nodes should not be matching any selectors (keep out of SelectorMatches?)
Matching milestone to dependent bug (it changed today to 0.9.1)
Target Milestone: mozilla0.9 → mozilla0.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
Target Milestone: mozilla0.9.1 → mozilla0.9.2
By mandate, moving non-crashers and non-dataloss bugs out.
Target Milestone: mozilla0.9.2 → mozilla1.0
Blocks: 104166
Taking -- I want to try to get this in and fix the dependency (and then work on
the issue hyatt mentioned).
Assignee: attinasi → dbaron
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0 → mozilla0.9.7
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
Attachment #55328 - Flags: review+
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;
 }
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.7 → mozilla0.9.6
Comment on attachment 55350 [details] [diff] [review]
more complete patch - prevent text nodes from being passed to style system

sr=attinasi
Attachment #55350 - Flags: superreview+
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
Attachment #55604 - Flags: superreview+
Comment on attachment 55604 [details] [diff] [review]
more complete patch -- prevent any style resolution on non-elements, including using pseudos

r=hyatt
Attachment #55604 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: