357614 - list of case sensitive HTML attributes for CSS attribute selector should be reversed

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

This is a followup to bug 356936.  The list of case sensitive HTML attributes for purposes of CSS attribute selector matching should probably instead be a list of case-insensitive attributes, so that any new attributes added are case sensitive by default.  See bug 356936 for more details.

Also, while we're there, we should probably also convert to |static const char caseSensitiveHTMLAttribute[][13]| for the reduction in shared library relocations.  (The space tradeoff is probably about even -- it makes all the strings take up 13 characters, but saves a pointer for each one.)

Comment 1

[Niels Leenheer (HTML5test)]

The list of case-insensitive attributes is as follows:

lang, dir, http-equiv, text, link, vlink, alink, compact, align, frame, rules, valign, scope, axis, nowrap, hreflang, rel, rev, charset, codetype, declare, valuetype, shape, nohref, media, bgcolor, clear, color, face, noshade, noresize, scrolling, target, method, enctype, accept-charset, accept, checked, multiple, selected, disabled, readonly, language, defer, type

Comment 2

[Niels Leenheer (HTML5test)]

Attachment: Patch against trunk

The default behavoir is case-sensitive. Only if the parser is set to case-insensitive AND the attribute is present in the list it is treated in a case-insensitive way.

Comment 3

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

Comment on attachment 243195 [details] [diff] [review]
Patch against trunk

Oops, I lost this since there was no review request.

Issues I need to look at:
 * I think we need to be more careful about namespace ID == None now -- we need to make sure it really only applies to HTML.
 * we should probably use static const char[][15] = { ... }

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, please don't treat all namespace-less elements as HTML elements. Call nsINode::IsNodeOfType(nsINode::eHTML) instead. (though i know in some parts of the selector matching code we artificially sets the namespace to kNameSpaceID_XHTML for such nodes, not sure if that happens here though).

Also, should this list apply to xhtml? Or is that always case sensitive?

Comment 5

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

So, we're already checking mCaseSensitive, so things aren't so bad -- although we really shouldn't be doing case sensitivity at parse time, but that's another bug.  We really want to check only kNameSpaceID_None, since we're looking at the namespace on the *attribute*.

Comment 6

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

Comment on attachment 243195 [details] [diff] [review]
Patch against trunk

Which means, in other words, that this patch is fine.  I'm just going to change this:

>+          if (mCaseSensitive == PR_FALSE) {  
>+            if (nameSpaceID == kNameSpaceID_None ||
>+                nameSpaceID == kNameSpaceID_XHTML) {

to use &&, which will means that there will be fewer indentation changes as well.

I'll also file a followup bug on removing the check for kNameSpaceID_XHTML there.

r+sr=dbaron with that change -- I'll check it in shortly

Comment 7

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

Patch checked in to trunk.  Thanks for the patch, and sorry for the huge delay.

Bug 282328 covers deferring making case-sensitivity decisions (at least, that's one of the options for it).

I filed bug 387615 as a followup on removing the kNameSpaceID_XHTML check.

Comment 8

[[e-mail]]

(In reply to comment #3)
>  * we should probably use static const char[][15] = { ... }
Surely 16 would be more efficient ;-)

Comment 9

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

I just added mozilla/layout/style/test/test_bug357614.html .