315620 - [FIX]:empty doesn't interact with CDATA sections or textnodes correctly

Status: RESOLVED FIXED

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

Description

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

According to the test, we should not consider CDATA sections to be something non-empty when they're empty.
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/20051019/xhtml/tests/css3-modsel-153.xml

We should double-check that this agrees with the latest css3-selectors spec.

I did check that this is not a dynamic change handling bug by switching body to display:none and then back again in DOM inspector.

Comment 1

[Hixie (not reading bugmail)]

I'm editing the spec as we speak. The test is correct here, the spec says:

# The :empty pseudo-class represents an element that has no children at all. In 
# terms of the DOM, only element nodes and text nodes (including CDATA nodes and 
# entity references) whose data has a non-zero length must be considered as 
# affecting emptiness; comments, PIs, and other nodes must not affect whether an 
# element is considered empty or not.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So...  What's the actual behavior we want for the various things we use that call IsSignificantChild?  At the moment, those are:

:first-child
:-moz-first-node
:last-child
:-moz-last-node
:only-child
:empty
:-moz-only-whitespace
:-moz-empty-except-children-with-localname()

That is, do we want to treat textnodes and cdata sections with 0 length as effectively equivalent to comments for all of these?  I _think_ we do...

If so, perhaps we should simply push up TextLength() to nsIContent so we don't have to QI to it every time we're looking at a textnode or cdata section here?  Either that or we just QI to nsITextContent even when white space is significant.  Shouldn't be that big a difference from what we have now, really.

Comment 3

[Mats Lindblad]

There is a testcase @ http://www.khanate.co.uk/empty.html that shows that :empty() doesn't work when applied to the body element.

Is this a bug or is it intentional?

Comment 4

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

That's bug 323052, not this bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 6

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

Comment on attachment 233532 [details] [diff] [review]
Fix

r+sr=dbaron

Comment 7

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

And, FWIW, this patch only affects :empty since :empty is the only case where |aWhitespaceIsSignificant| is true, and |!aChild->TextIsOnlyWhitespace()| implies |aChild->TextLength() != 0|.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fix checked in.  Still need to land a regression test.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Regression test

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in the test.