Bug 15470 - Make attr selectors case-sensitive for case-sensitive HTML attrs (Acid3 bug)
Summary: Make attr selectors case-sensitive for case-sensitive HTML attrs (Acid3 bug)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.css3.info/selectors-test/
Keywords:
: 11389 16703 (view as bug list)
Depends on:
Blocks: 11390
  Show dependency treegraph
 
Reported: 2007-10-11 17:22 PDT by Eric Seidel (no email)
Modified: 2008-01-28 09:16 PST (History)
5 users (show)

See Also:


Attachments
Fix (test updates forthcoming) (4.69 KB, patch)
2007-10-11 17:28 PDT, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Case sensitivity fix, including testcase (19.08 KB, patch)
2008-01-06 04:36 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-10-11 17:22:13 PDT
Make attr selectors case-sensitive for case-sensitive HTML attrs

currently we fail 18 tests in this suite:
http://www.css3.info/selectors-test/

because of this "bug".  The justification is that the CSS2 spec says that case sensitivity for values (other than element names) is "language dependent":

http://www.w3.org/TR/css3-selectors/#attribute-selectors

And HTML has certain attributes which are case sensitive and others which are not:
http://www.w3.org/TR/REC-html40/types.html#h-6.1

Strangely enough, both Opera and FireFox pass different subsets of these 18 tests.  They seem to be related to implementation details in both browsers.  Given that neither of them fail all 18 like we do (but rather seem to attempt to support this strangeness), I think this patch is correct.
Comment 1 Eric Seidel (no email) 2007-10-11 17:28:29 PDT
Created attachment 16636 [details]
Fix (test updates forthcoming)
Comment 2 Maciej Stachowiak 2007-10-11 23:43:53 PDT
I think it's worth testing this in both quirks mode and standards mode in other browsers (especially Firefox and IE).
Comment 3 Darin Adler 2007-10-15 09:16:23 PDT
Comment on attachment 16636 [details]
Fix (test updates forthcoming)

htmlAttributeHasCaseSensitiveValue should probably use a set instead of 18 separate calls. Can first check that there's no namespace, then use a set of AtomicString for the attr names.

+            if (caseSensitive && sel->m_value != value)
+                return false;
+            else if (!caseSensitive && !equalIgnoringCase(sel->m_value, value))
                 return false;

No need to else after return.

I'd write it with ? : instead:

    if (!(caseSensitive ? sel->m_value == value : equalIgnoringCase(sel->m_value, value)))
        return false;

I think it's a bit unfortunate that we're going to be computing the caseSensitive boolean all the time. Does every case in the switch statement use it?

review- for the set issue
Comment 4 Eric Seidel (no email) 2008-01-01 22:09:54 PST
*** Bug 16703 has been marked as a duplicate of this bug. ***
Comment 5 Eric Seidel (no email) 2008-01-01 22:10:35 PST
it is unlikely I'll get to this patch again anytime soon.
Comment 6 Eric Seidel (no email) 2008-01-06 04:36:48 PST
Created attachment 18300 [details]
Case sensitivity fix, including testcase

 LayoutTests/ChangeLog                              |   11 ++
 .../css/html-attr-case-sensitivity-expected.txt    |  129 ++++++++++++++
 .../fast/css/html-attr-case-sensitivity.html       |   13 ++
 .../css/resources/html-attr-case-sensitivity.js    |  176 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   15 ++
 WebCore/css/CSSStyleSelector.cpp                   |   84 +++++++++-
 6 files changed, 420 insertions(+), 8 deletions(-)
Comment 7 Darin Adler 2008-01-06 11:25:51 PST
Comment on attachment 18300 [details]
Case sensitivity fix, including testcase

+// test a non-existant attr

This should say "nonexistent", not "non-existant".

r=me
Comment 8 Eric Seidel (no email) 2008-01-06 12:35:40 PST
Yay!  we now pass the 18 css3 selector tests related to this bug!
r29212
Comment 9 Robert Blaut 2008-01-28 08:44:32 PST
Please close a bug 11389 as a duplication of this bug.
Comment 10 Robert Blaut 2008-01-28 08:47:50 PST
Also add this bug to meta bug 11390.
Comment 11 Alexey Proskuryakov 2008-01-28 09:15:43 PST
*** Bug 11389 has been marked as a duplicate of this bug. ***