Closed Bug 157395 Opened 23 years ago Closed 20 years ago

:empty should not match <foo> </foo>

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8alpha6

People

(Reporter: bbaetz, Assigned: Callek)

References

(
URL
)

Details

(Keywords: css3, testcase)

Attachments

(1 file, 5 obsolete files)

(v1.3) Address review comments, and -pu8
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
The css3 selectors spec says that :empty "represents an element that has no children at all, including possibly empty text nodes" The testcase from the wc3 tests back this up. I've got a patch I'll attach in a sec. With this patch we don't regress any of the static :empty cases (the dynamic ones don't work; theres another bug on that) ian, are there more tests you have for :empty? ian, dbaron, can I get r/sr on this?
Attached patch patch (obsolete) (deleted) — — Splinter Review
With this patch we just check for the existence of children, not whether they are 'significant'
Status: NEW → ASSIGNED
Keywords: css3, patch, review, testcase
Target Milestone: --- → mozilla1.1beta
Note that there is a side effect to this. Bug 97361 added a quirk to collapse the top and bottom margins of empty elements. That fix uses :empty to do the matching, which means that: <html><head><title>Testcase for bug 97361</title></head> <body topmargin="0" marginheight="0"> <p> </p> <table border=1> <tbody> <tr> <td>Top</td> </tr> </table> </body></html> will (will this fix) show an empty line before the table. ns4 doesn't do this. Is this something we need to worry about? The test page from that bug looks the same, and the testcase didn't have whitespace within the <p></p>
You have to ignore comment nodes, processing instructions, etc.
The selectors spec says: "The :empty pseudo-class represents an element that has no children at all, including possibly empty text nodes, from a DOM point of view." The DOM inspector shows comment/PI nodes; doesn't that mean that an element containing a comment does not have 'no children at all', and so :empty shouldn't match if there is a comment? I'm not sure what 'from a DOM point of view' is meant to mean. DOM2-core includes comments/PI nodes, though. What did you mean by 'etc'? The current code ignores text/comment/pi only for this sort of thing - is there something else ?
Oh, my bad, now that I think back on it we decided to take comments into account. Never mind.
It sounds like we need to find another workaround for bug 97361.
(Perhaps that would be a proper implementation of "special" margins so we don't have to keep adding zero margins as we want to get more compatible.)
I think <foo> </foo> should not be considered empty. simple test case: <p>hello<span style="font-size:5em;"> </span>world. hello world</p> (<span> </span> definitely contains something)
yes, thats what this patch does.
Reconfirmed using FizzillaCFM/2002071508. Setting All/All.
OS: Linux → All
Hardware: PC → All
Blocks: 75186
bz told me that we wanted to do this in standard compliant rendering only if dbaron agreed. I was wondering how the patch should be modified to reflect that, something like: if(scr){ element->ChildAt(0, child); } else{ // the code that the patch currently erases } Not entirely sure though, but if someone could give me some directions, I could create a patch and maybe find someone who wants to test it for me :-)
Anne, just look at what the arguments to IsSignificantChild mean, please. In standards mode, I believe we'd just want to pass PR_FALSE. We can't just do the ChildAt() thing, since we still have to ignore various nodes (comments, etc) per spec.
A bit harder than I hoped due to Bug 97361, there should be a fix, but not as easy as I'd have hoped.
Bug 97361 touches only quirk.css.. http://lxr.mozilla.org/mozilla/source/layout/base/src/nsPresContext.cpp#800 Shows that we only use quirk.css (as expected) when we are in quirks mode, not otherwise. The only other place that seems we use :empty in our internal code is http://lxr.mozilla.org/mozilla/source/editor/composer/src/res/EditorOverride.css#60 Where it does seem to be using the quirky version, at a glance, though I dont know _that_ code well, though the blame implies that standards :empty wont break anything. My patch I started introduces a new PseudoClass, already, and changes all involved, for clarity, though I can revert to "all-standards" Pseudo, and the blocking bug should not have any problems due to eCompatibility_NavQuirks being set. Bz, Or Dbaron comment on what you think I should do?
Assignee: bbaetz → 116057
Status: ASSIGNED → NEW
Attached patch v0.9 (obsolete) (deleted) — — Splinter Review
Version 0.9, does the extended additions, if this is agree'd as the way we should go, I'll flag r/sr's Otherwise I can fix it for a new patch, (currently un-tested, but here for anyone's eyes who wants to look)
Attachment #91269 - Attachment is obsolete: true
:empty shouldn't be quirky, ever. Only :-moz-quirky-empty (or whatever it's called). Might be better to call it :-moz-only-whitespace or something.
Comment 16 is my sentiment exactly.
Status: NEW → ASSIGNED
Attached patch add :-moz-only-whitespace, fix :empty (obsolete) (deleted) — — Splinter Review
This makes current behavior use :-moz-only-whitespace and fixes behavior of :empty as detailed
Attachment #166917 - Attachment is obsolete: true
Attached patch v1.1 Adds :-moz-only-whitespace as well (obsolete) (deleted) — — Splinter Review
This also patches IsSignificantChild to allow what I need, bz agree'd with the new argument to IsSignificantChild in IRC
Attachment #167404 - Attachment is obsolete: true
Attached patch v1.2 (obsolete) (deleted) — — Splinter Review
This should be what the last patch was intended to be (I took the diff a bit early it seems).
Attachment #167615 - Attachment is obsolete: true
Comment on attachment 167710 [details] [diff] [review] v1.2 As I said, anyplace in current tree which used :empty, I defered to the newly added :-moz-only-whitespace, just to be safe.
Attachment #167710 - Flags: superreview?
Attachment #167710 - Flags: review?
Attachment #167710 - Flags: superreview?(bzbarsky)
Attachment #167710 - Flags: superreview?
Attachment #167710 - Flags: review?(bzbarsky)
Attachment #167710 - Flags: review?
Whiteboard: [reviewsPending]
Target Milestone: mozilla1.1beta → mozilla1.8alpha6
Comment on attachment 167710 [details] [diff] [review] v1.2 Please use the -p argument to diff and include more context; that makes the diff a lot easier to review. -pu8 is a good way to diff. >Index: content/html/style/src/nsCSSStyleSheet.cpp >+static PRBool IsSignificantChild(nsIContent* aChild, PRBool aAcceptText, PRBool aIsWhitespaceSignificant) I think it makes sense to rename aAcceptText to aTextIsSignificant. The last arg would be better called aWhitespaceIsSignificant You probably want: NS_ASSERTION(!aWhitespaceIsSignificant || aTextIsSignificant, "Nonsensical arguments"); (because it makes no sense to pass aWhitespaceIsSignificant true, but aTextIsSignificant false). >+ if (aAcceptText) { >+ if (tag == nsLayoutAtoms::textTagName) { Just use a single if() with an && between the two conditions here. >+ if (!aIsWhitespaceSignificant){ // skip only whitespace text This function isn't skipping. It's saying whether a child is significant. And you need a space before the '{'. >+ }else{ Spaces around the "else", please. And prevailing style for this file is: if () { } else { } >+ PRBool isWhitespaceSignificant = PR_TRUE; > PRInt32 index = -1; >+ >+ if (nsCSSPseudoClasses::mozOnlyWhitespace == pseudoClass->mAtom) >+ isWhitespaceSignificant = PR_FALSE; Just follow the example of the other branches here: PRBool isWhitespaceSignificant = nsCSSPseudoClasses::mozOnlyWhitespace == pseudoClass->mAtom); With those nits fixed, this should be good to go.
Attachment #167710 - Flags: superreview?(bzbarsky)
Attachment #167710 - Flags: superreview-
Attachment #167710 - Flags: review?(bzbarsky)
Attachment #167710 - Flags: review-
Attachment #167710 - Attachment is obsolete: true
Attachment #167716 - Flags: superreview?(bzbarsky)
Attachment #167716 - Flags: review?(bzbarsky)
Comment on attachment 167716 [details] [diff] [review] (v1.3) Address review comments, and -pu8 r+sr=bzbarsky
Attachment #167716 - Flags: superreview?(bzbarsky)
Attachment #167716 - Flags: superreview+
Attachment #167716 - Flags: review?(bzbarsky)
Attachment #167716 - Flags: review+
Patch Checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [reviewsPending]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: