If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dbaron, Assigned: bz)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

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.
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.
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.
Summary: :empty doesn't interact with CDATA sections correctly → :empty doesn't interact with CDATA sections or textnodes correctly

Comment 3

12 years ago
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?
That's bug 323052, not this bug.
Created attachment 233532 [details] [diff] [review]
Fix
Assignee: dbaron → bzbarsky
Status: NEW → ASSIGNED
Attachment #233532 - Flags: superreview?(dbaron)
Attachment #233532 - Flags: review?(dbaron)
Priority: -- → P3
Summary: :empty doesn't interact with CDATA sections or textnodes correctly → [FIX]:empty doesn't interact with CDATA sections or textnodes correctly
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 233532 [details] [diff] [review]
Fix

r+sr=dbaron
Attachment #233532 - Flags: superreview?(dbaron)
Attachment #233532 - Flags: superreview+
Attachment #233532 - Flags: review?(dbaron)
Attachment #233532 - Flags: review+
And, FWIW, this patch only affects :empty since :empty is the only case where |aWhitespaceIsSignificant| is true, and |!aChild->TextIsOnlyWhitespace()| implies |aChild->TextLength() != 0|.
Fix checked in.  Still need to land a regression test.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Created attachment 248687 [details] [diff] [review]
Regression test
Checked in the test.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.