53974 - Text nodes are getting :after pseudo-elements

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

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

DESCRIPTION:  Eek.  Moving the mouse around in the test page
http://www.people.fas.harvard.edu/~dbaron/css/test/pseudos causes lots of extra
content to be generated.

STEPS TO REPRODUCE:
 * load http://www.people.fas.harvard.edu/~dbaron/css/test/pseudos
 * move the mouse around

ACTUAL RESULTS:
 * The paragraph that starts with the generated content "Beginning of an
Element" gets some of its non-generated content duplicated repeatedly!

EXPECTED RESULTS:
 * content stays the same, since the page is completely static

BUGGY IN:
 * MacOS 9.0.4 Mozilla 2000-09-22-20

Comment 1

[karnaze (gone)]

Reassigning to Buster.

Comment 2

[buster]

I see this on WinNT as well.
Problems of this sort are often block-in-inline issues.  cc-ing waterson.
david, can you generate a smaller test case?
marking P1: data corruption, page unusable.
holding off on nominating for rtm until we understand the problem better

Comment 3

[buster]

chris said he'd take a look, thanks chris.

Comment 4

[Hixie (not reading bugmail)]

Attachment: Stylesheet for next test case

Comment 5

[Hixie (not reading bugmail)]

Attachment: Test case by [e-mail]. Hover over the content to see the bug.

Comment 6

[Hixie (not reading bugmail)]

Wow, this is scary. This is a must fix for RTM, it just ruins perfectly ordinary
HTML and XML pages. Yikes!  

[e-mail]: Thanks for the test case!

Comment 7

[Chris Waterson]

I'm not seeing the problem in 2000-10-04-08; however, this could be because
joki's :hover stuff has been disabled? (Somebody remind me how to turn that on...)

Comment 8

[Hixie (not reading bugmail)]

Actually, the bug is visible in the second testcase (still) but doesn't need
mouse movement. There should only be two instances of -X-, but we display three.

What's the opposite of the 'dataloss' keyword? We need it for this bug...

Comment 9

[Hixie (not reading bugmail)]

Er. There should only be _one_ instance of -X-, but we display three. As David
just pointed out to me. Sorry about that.

Could the problem be that we are giving text nodes :after content? (Guessing...)

Comment 10

[Chris Waterson]

Attachment: simplified test case; expect "oneXtwothree", not "XoneXtwoXthree"

Comment 11

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

For a possibly related bug (and possibly a historical curiosity), see bug 4834.

Comment 12

[Hixie (not reading bugmail)]

Attachment: I was right, we're matching on text nodes...

Comment 13

[Hixie (not reading bugmail)]

Yep, we're matching on text nodes. The testcase I just attached uses the same
stylesheet as the second attachment (namely, the first attachement) which reads:

   root > *:after { content: '-X-'; }

Here is the test case:

   <root>  <!-- --> <!-- --> <!-- --> <!-- --> <!-- --> <!-- -->  </root>

Guess how many -X-s there are... that's right, 7, as many as there are text 
nodes in that document. There should be none.

This should be an easy fix -- nothing, but NOTHING, ever matches a text node in
CSS. Only elements and pseudo-elements can match a selector. And text nodes
should not get :before and :after pseudo-elements, only elements get those.

Buster's work in making leaf elements do generated content is a prime candidate
for having caused this regression... Buster?

Comment 14

[buster]

I will look at this.  Great work, folks.

Comment 15

[Chris Waterson]

Actually, I doubt it was buster's fault, although it might be related to r1.505
of nsCSSFrameConstructor.cpp, or the followup fix in r1.511. I'll attach a
skanky patch that'll fix the bug at a slight performance cost. If the spirit of
the fix is correct, we should probably hoist it.

Comment 16

[Chris Waterson]

Attachment: one way to make text not match selectors

Comment 17

[buster]

Attachment: here's another way: shut off :before and :after on text nodes in nsCSSFrameConstructor.cpp

Comment 18

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

Are there any other types of nodes that aren't element nodes that could possibly 
have :before and :after mistakenly attached to them (e.g., entity reference 
nodes)?  Is there an easy way to instead check that what you have *is* an 
element?  (GetNodeType?)

Comment 19

[buster]

talked with marc.  his first impression was that my fix was potentially safer,
but waterson's was potentially a big performance win as well as being at least
as correct as mine.  I'll spend some time testing waterson's fix (with my code
removed) to convince myself it's equally safe.

Comment 20

[Chris Waterson]

Uh, I think my code would probably be a net loss. It's my guess that your
changes to allow leaf elements to have pseudos was the first time this code got
tripped. Why don't you try this instead:

- Use your patch
- Make SelectorMatches() assert if it ever gets handed
  a text node as aContent. In theory, it shouldn't.

That way we're sure nobody's trying to match text nodes.

Comment 21

[buster]

Chris: can't assert in SelectorMatches if it gets handed a text node.  this
happens all the time.  it's basically a no-op, but the assert would require we
fix other places first and I don't want that kind of fan-out from this bug.
Let's stick to the problem at hand, and go with my fix.

Comment 22

[Chris Waterson]

Why does SelectorMatches() get handed text nodes all the time? That seems like a
bug to me, based on ianh's comments above: "nothing, but NOTHING, ever matches a
text node in CSS".

I'm not averse to cut-and-burn hackery on the branch, but let's try to
understand this problem on the trunk. Or, maybe it's not a problem, and you can
tell me why, and I'll shut up. :-)

Comment 23

[Hixie (not reading bugmail)]

And as David said, nothing in CSS ever matches comment nodes, entity nodes,
doctypes, CDATA nodes, processing instructions, or attributes (god forbid)
either. So maybe we should just check if we are matching an element instead of
checking whether we are not matching a text node?

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Maybe I misunderstood what Hixie said, but CSS does have attribute selectors:

http://www.w3.org/TR/REC-CSS2/selector.html#attribute-selectors

Comment 25

[Hixie (not reading bugmail)]

Yes (and we support them) but they don't select the attributes, they merely
select the elements whose attributes meet the conditions. It is still the 
element that matches, and not the attributes.

Comment 26

[buster]

harsh reality time: we're out of time for further discussions if this is going
to make the NS6 train.
so, for the patch I attached...marc, r=? waterson, a=?  or should we rtm- it?  I
think that would be a shame at this point.  I would suggest just checking this
in on the branch, and doing the more complete solution on the trunk post rtm.

Comment 27

[Chris Waterson]

Sure, I'd a= the 10/10/00 11:15 patch for both the branch and trunk.

Comment 28

[Marc Attinasi]

Steve, your patch is fine. r=attinasi for that, get it in.

For the long-term, I'm opening a new bug to remind me to look at why
SelectorMatches is getting called on text nodes, and see if it makes more sense
to keep them out of SelectorMatches or make SelectorMatches handle them better,
ala Waterson's change.

Comment 29

[Marc Attinasi]

Opened bug 56117 to look at the best way to handle text nodes in SelectorMatches...

Comment 30

[karnaze (gone)]

Marking rtm+.

Comment 31

[selmer (gone)]

rtm-, can't even repro this (using win95)

Comment 32

[Hixie (not reading bugmail)]

selmer, pdt: To reproduce, view:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=16579
The page should be blank, however there are seven -X- marks.

Not fixing this means that web authors will be totally unable to use the CSS2
generated content feature for as long as N6 is on the market. To put this in 
perspective: this feature has a whole chapter dedicated to it in the spec. We 
are not talking about a minor blip here.

In addition, considering the root cause, it is likely that there are other
errors that could be occuring but that we have not yet spotted. The fix should
prevent any of these potential errors too.

Also, when this bug was reported there was an even more serious problem
whereby merely hovering over content caused it to be duplicated. We do not want
this to regress from under us when/if whatever caused that is changed again.

Renominating in light of above summary.

Comment 33

[Phil Peterson]

Considering that this doesn't crash or hang, and no popular web sites are
unreadable due to this bug, I can't see that we're Breaking The Web. Marking
[rtm-] again. Time to focus on crash and data loss bugs.

Comment 34

[buster]

this change is so small and safe, and the fix so obvious, that I understand
ian's frustration.  if we advertise that we support this feature, but we support
it badly in a way that violates the spec (thou shall not match text nodes) and
in a way that the author can't easily code around, it really makes us look bad.

I also understand the pdt's position.  Ian overstates his case somewhat:  before
and after does work in many situations just fine. It's only a subset of the
types of rules an author might use that could trigger this problem.  However, it
won't be obvious to authors what those cases are.

Coding has to be shut down sometime.  I understand the desire to have us work on
crasher bugs, but when work on this bug started it looked *very* serious (the
hover bug ian mentioned.)  So given that the work and testing are done, it's a
shame not to take the fix.

ekrock: any thoughts on the matter?

Comment 35

[Chris Waterson]

I strongly believe that we should get this fix in. Note that hyatt had to back
out his :hover fixes, so this is likely to exacerbate that problem. As buster
notes, this bug is extremely low risk: "if you're about to add a pseudo-class to
a text node, don't!"

Comment 36

[ekrock's old account (dead)]

Marking rtm+ to trigger re-evaluation. PDT: It is true that this problem doesn't 
break lots of top100 pages today, but the bug is a critical problem from another 
standpoint: granted we aren't Breaking The Web of Today, but we're building in a 
blocker bug into N6 that will prevent content authors from being able to use 
this feature in many cases (the ones that cause the behavior) until N6's market 
share has been largely replaced by N6+'s. Moreover, the unreliability of the 
feature that this bug creates will deter content providers from attempting to 
use the feature even in the cases that don't trigger the bug because they will 
have no way to tell which cases are which in advance. 

So this bug is Breaking The Web of Tomorrow and obstructing the ability of 
content authors to adopt the features of the specification. Enabling the usage 
of standards is one of the key goals of this release. Avoiding the creation of 
booby traps that block adoption of standards features is important to make that 
possible.

Given that reality, I think it's important to exercise the rule of reason here 
and evaluate risk-benefit rather than blindly applying some criterion. We're 
managers; we're paid to exercise judgment. We know the patch is extremely low 
risk: we simply check whether we're about to apply this behavior in the case we 
shouldn't (on a text node), and if we are, we do nothing. There is no case in 
which this behavior *should* be applied to a text node, so we are simply 
introducing a sanity-check to the code for a case known to be 100% wrong. It is 
difficult to conceive of how this could cause a regression in the code.

Ian, Chris: We will have to make this case in person. Please join me Friday at 
4:00 in Star Trek. Steve: please be available so we can dial you in. Thanks!

Comment 37

[Phil Peterson]

rtm++

Comment 38

[buster]

thanks.  I'll get this in late tonight.

Comment 39

[buster]

fixed on branch

Comment 40

[buster]

fixed on trunk as well.

Comment 41

[Asa Dotzler [:asa]]

Verified Fixed on Mozilla trunk builds.  Adding vbranch keyword because I don't
see any evidence of a branch verification.

linux 102608 RedHat 6.2
win32 102604 NT 4
mac 102608 Mac OS9

removing vtrunk keyword and replacing it with vbranch

Comment 42

[Christine Hoffman]

Using 10/26 branch build, verified fixed on the duplication issue. There are 
still problems with the original testcase (written up in another bug).