507762 - Parse CSS stylesheets independently of case-sensitivity

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Zbarsky (:dzbarsky)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

.

Reproducible: Always

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Work in progress

This patch fixes the following testcases:
https://bug429298.bugzilla.mozilla.org/attachment.cgi?id=315958
https://bug370513.bugzilla.mozilla.org/attachment.cgi?id=255224

It still fails
https://bug429298.bugzilla.mozilla.org/attachment.cgi?id=315960

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 3

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

That list of which attributes are case sensitive has to go somewhere (or, actually, probably, stay where it is); otherwise you'll be regressing what it does.  (I hope we have a test for that... if not, we probably need to add one.)  So attribute selectors need to keep their mCaseSensitive -- but its meaning should change from "is the value case sensitive?" to "is the value case sensitive if we're in an HTML document?"... which means you actually need to add a drop of code to AttrMatchesValue, not remove.

Attribute selectors are a bit confusing, since whether the attribute name is case sensitive and whether the attribute value is case sensitive are two different decisions.  (The combination of having a case-sensitive name and a case-insensitive value never occurs, but the other three combinations do.)

Also, you need to do something about the code that handles attr() (which is a value of the 'content' property; that's what the ParseAttr changes are); you've removed the lowercasing, but you need to add it back at the other end if you're in a document that requires it.  I think the code in question is in nsCSSFrameConstructor::CreateGeneratedContent .


We probably ought to have tests for correct case handling of attribute selectors (both attribute names and values) and of attr().



I haven't looked at the rest in incredible detail yet, although I did notice a few "if(" instead of "if (".

Also:
+  // Now  reset the compatibility mode to match our compat mode.
No double-space, and probably add back "of the CSSLoader" from the first part of the comment that you removed.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch 1.1

Comment 5

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

Comment on attachment 392360 [details] [diff] [review]
Patch 1.1

>@@ -1701,16 +1702,19 @@ nsCSSFrameConstructor::CreateGeneratedCo

>+      if (!mDocument->IsCaseSensitive())
>+        ToLowerCase(contentString);

This should also check whether it's an HTML node.

>-          aDataMask |= SEL_MASK_ATTRIB;
>-          aSelector.AddAttribute(nameSpaceID, attr, func, value, isCaseSensitive);
>+           aDataMask |= SEL_MASK_ATTRIB;
>+           aSelector.AddAttribute(nameSpaceID, attr, func, value, isCaseSensitive);

No need to make the indent wrong. :-)

> static PRBool AttrMatchesValue(const nsAttrSelector* aAttrSelector,
>-  const nsStringComparator& comparator = aAttrSelector->mCaseSensitive
>+  const nsStringComparator& comparator = (aAttrSelector->mCaseSensitive && !isHTML)

Should this be || ?

And any chance I could talk you into adding tests that would have caught that mistake?  (And also tests for the other case sensitivity issues here?)

Comment 6

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

Comment on attachment 392360 [details] [diff] [review]
Patch 1.1

See comment 5.

Comment 7

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

Any chance of an updated patch here?

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Patch 1.2

This failed a few tests on try-server, but I couldn't reproduce it locally and I think they're unrelated.

Comment 9

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

Different tests on each platform?

Comment 10

[David Zbarsky (:dzbarsky)]

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249443390.1249449876.30339.gz#err0
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1249447283.1249453635.6688.gz#err0

Comment 11

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

Comment on attachment 397022 [details] [diff] [review]
Patch 1.2

>       if (-1 != barIndex) {
>         nsAutoString  nameSpaceVal;
>         contentString.Left(nameSpaceVal, barIndex);
>         PRInt32 error;
>         attrNameSpace = nameSpaceVal.ToInteger(&error, 10);
>         contentString.Cut(0, barIndex + 1);
>         if (contentString.Length()) {
>+           if (mDocument->IsHTML() && attrNameSpace == kNameSpaceID_None)
>+             ToLowerCase(contentString);
>           attrName = do_GetAtom(contentString);
>         }
>       }
>       else {
>+        if (mDocument->IsHTML())
>+            ToLowerCase(contentString);
>         attrName = do_GetAtom(contentString);
>       }

You should {} the blocks after the if.

Also don't over-indent the second one.

And, as I said before, don't you need to check that this is an HTML *element*?  (SVG elements in HTML documents should be case sensitive, right?)

And maybe add a test for this case?

>-  const nsStringComparator& comparator = aAttrSelector->mCaseSensitive
>+  const nsStringComparator& comparator = (aAttrSelector->mCaseSensitive || !isHTML)
>                 ? static_cast<const nsStringComparator&>(defaultComparator)
>                 : static_cast<const nsStringComparator&>(ciComparator);

What about a test for the bit you got wrong here in the previous patch?

>+		matchAttribute = isHTML ? attr->mLowercaseAttr : attr->mCasedAttr;

This has a TAB character and is therefore incorrectly indented.  Use spaces rather than tabs.

So do a few other things later in the patch.

>-              AttrValueIs(attr->mNameSpace, attr->mAttr, attr->mValue,
>-                          attr->mCaseSensitive ? eCaseMatters : eIgnoreCase);
>+            AttrValueIs(attr->mNameSpace, matchAttribute, attr->mValue,
>+                        (isHTML && attr->mCaseSensitive) ? eCaseMatters : eIgnoreCase);

Hmmm... it seems I missed this the previous round; do you have a test for this?


I think the nsBox.cpp changes were supposed to be part of something else.  They also introduce a bunch of TAB characters.

Comment 12

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

The crashtest failure after 505912-1 isn't something I recognize as random orange, and it happened in both; could that be your problem?

Comment 13

[Anne (:annevk)]

FWIW, there was a proposal of removing the case-insensitive attribute list from HTML5 treating them like any other attribute effectively. Currently it's just a somewhat arbitrary fixed list that is not useful to developers and only adds footprint and complexity to implementations.

I think what is blocking removal currently is implementations dropping it.

Comment 14

[David Zbarsky (:dzbarsky)]

> And, as I said before, don't you need to check that this is an HTML *element*? 
> (SVG elements in HTML documents should be case sensitive, right?)
> 
> And maybe add a test for this case?
> 
Working on it

> What about a test for the bit you got wrong here in the previous patch?
There are tests for that.

> Hmmm... it seems I missed this the previous round; do you have a test for this?
There are already tests for this.

> 
> I think the nsBox.cpp changes were supposed to be part of something else.  They
> also introduce a bunch of TAB characters.
Yes, that's something else.  Sorry

Boris says: Seems to have passed tests on Linux.  Had 2 xpcshell test fails on Mac, but your patch wouldn't have affected those.

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Patch with tests

Comment 16

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

Comment on attachment 397485 [details] [diff] [review]
Patch with tests

Sorry for the delay is getting to this; I was traveling for a few days
and then catching up from that (though I really should have gotten to
this yesterday).


+        if (mDocument->IsHTML() && aParentContent->IsNodeOfType(nsINode::eHTML))
+          ToLowerCase(contentString);

Could you brace this if as well?

>           // If we're parsing a style sheet for an HTML document, and
>           // the attribute selector is for a non-namespaced attribute,
>           // then check to see if it's one of the known attributes whose
>           // VALUE is case-insensitive.

This comment needs to be changed from "If we're parsing a style sheet
for an HTML document" to "For cases when this style sheet is applied to
an HTML element in an HTML document".


>-            if (mCaseSensitive) {
>               attr.Append(mToken.mIdent);
>-            } else {
>-              nsAutoString buffer;
>-              ToLowerCase(mToken.mIdent, buffer);
>-              attr.Append(buffer);
>-            }

>-          if (mCaseSensitive) {
>             attr = holdIdent;
>-          }
>-          else {
>-            ToLowerCase(holdIdent, attr);
>-          }

>-          if (mCaseSensitive) {
>             attr.Append(mToken.mIdent);
>-          } else {
>-            nsAutoString buffer;
>-            ToLowerCase(mToken.mIdent, buffer);
>-            attr.Append(buffer);
>-          }

In all three of these cases, you need to fix the indentation of the one
line you're keeping.

>-    //If we tested that this is an HTML node in a text/html document and
>-    //had some tweaks in RuleHash, we could remove case-sensitivity from
>-    //style sheets.
>-    if (data.mIsHTMLContent) {
>-      if (data.mContentTag != aSelector->mLowercaseTag)
>-        return PR_FALSE;
>+
>+    PRBool bail = PR_TRUE;
>+
>+    if (isHTML) {
>+      if (data.mContentTag == aSelector->mLowercaseTag)
>+        bail = PR_FALSE;
>     }
>-    else {
>-      if (data.mContentTag != aSelector->mCasedTag)
>-        return PR_FALSE;
>-    }
>+    else
>+      if (data.mContentTag == aSelector->mCasedTag)
>+        bail = PR_FALSE;
>+
>+    if (bail)
>+      return PR_FALSE;

I don't see why you're introducing the |bail| local variable.  Why not
just change the |data.mIsHTMLContent| in the existing code to |isHTML|
and remove the comment?

In fact, you could even simplify it using:
nsIAtom *matchTag =
  isHTML ? aSelector->mLowercaseTag : aSelector->mCasedTag;

>             data.mContent->
>-              AttrValueIs(attr->mNameSpace, attr->mAttr, attr->mValue,
>-                          attr->mCaseSensitive ? eCaseMatters : eIgnoreCase);
>+            AttrValueIs(attr->mNameSpace, matchAttribute, attr->mValue,
>+                        (isHTML && attr->mCaseSensitive) ? eCaseMatters : eIgnoreCase);

The last line should be:

  (!isHTML || attr->mCaseSensitive) ? eCaseMatters : eIgnoreCase);

And could you add a test for that case?  (i.e., make sure that for
non-HTML content attributes match case-sensitively whether or not
they're in the list?) Sorry I missed that one the last time around.

Also, could you leave the indent of AttrValueIs?


>-    new nsAttrSelector(mNameSpace, mAttr, mFunction, mValue, mCaseSensitive);
>+    new nsAttrSelector(mNameSpace, mLowercaseAttr, mCasedAttr, mFunction, mValue, mCaseSensitive);

This needs to wrap at less than 80 characters.

> // -- nsCSSSelector -------------------------------
>-
> nsCSSSelector::nsCSSSelector(void)

Better to leave that blank line.



Sorry I keep finding things I missed the last time around; hopefully
this is the time, though.

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

Comment 18

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

Hmmm.  It looks like you managed to insert some more TAB characters around two of the changes I requested in the previous round (search the patch for tabs to find them).  You also managed to change the newlines in all the reftests from Unix newlines (LF) to Windows ones (CR-LF); that's incorrect, since the source should use Unix newlines.

In SelectorMatches, these lines:

>+  const PRBool isHTML = data.mIsHTMLContent && data.mContent->GetOwnerDoc()->IsHTML();

>+    (isHTML ? aSelector->mLowercaseTag : aSelector->mCasedTag) != data.mContentTag) {

are too long, and should be wrapped at under 80 characters.

Also, in the second of them, the ( should line up with the "a" in the line before it so that both halves of the && are at the same indentation.

This line:
+          // For cases when this style sheet is applied to
ended up with a carriage return at the end (i.e., CR-LF instead of just LF) at the end.  Also, the text in the rest of the comment should be rewrapped appropriately.

Also, a bunch of changes to nsSliderFrame.cpp snuck in that shouldn't be part of this patch.


Also, it looks like 507762-1-ref.html and 507762-2-ref.html are the same.  If they are, you may as well not have both (and just adjust the reftest.list).  But maybe I'm not noticing a difference.



In any case, r=dbaron with those issues fixed.  If you want me to update the patch for you, let me know.

Comment 19

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

I addressed those comments and landed the patch:
http://hg.mozilla.org/mozilla-central/rev/2c6aeb49c93c

Comment 20

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the tests here tested attribute matching, but not HasAttributeDependentStyle in the mixed-case HTML case....