Closed Bug 357614 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: css2, helpwanted, html4)

Attachments

(1 file)

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.)
Keywords: helpwanted
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
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 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] = { ... }
Attachment #243195 - Flags: review?(dbaron)
Flags: blocking1.9?
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?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
QA Contact: ian → style-system
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 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
Attachment #243195 - Flags: superreview+
Attachment #243195 - Flags: review?(dbaron)
Attachment #243195 - Flags: review+
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #3)
>  * we should probably use static const char[][15] = { ... }
Surely 16 would be more efficient ;-)
I just added mozilla/layout/style/test/test_bug357614.html .
Flags: in-testsuite+
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Depends on: 456341
You need to log in before you can comment on or make changes to this bug.