820902 - nsAttrValue::Equals should do ASCII-case-insensitive compares

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now it does nsCaseInsensitiveStringComparator, which is all wacky.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Testcase at http://people.mozilla.org/~jdaggett/tests/casesensitivity-classid.html

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: nsAttrValue::Equals should do ASCII-case-insensitive compares when it's doing case-insensitive compares.

Comment 3

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f91a854410

Comment 4

[:Benjamin Peterson]

Backed out for causing https://tbpl.mozilla.org/php/getParsedLog.php?id=17920584&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/a17cb882eb6b

Comment 5

[Boris Zbarsky [:bzbarsky]]

Looks like at least some of that test expects the Ü key to match the ü accesskey or vice versa.

I just went through all the consumers of eIgnoreCase with AttrValueIs, FindAttrValueIn, and nsAttrValue::Equals, and most of them just pass in an ASCII literal or atom to compare to.  The exceptions are:

1) nsDocument::FindImageMap.  Per spec this needs to be doing Unicode "compatibility
   caseless" matching at least in some cases, so it doesn't want the ASCII behavior
   anyway.
2) IsAccessKeyTarget in the ESM.  Per spec I think this should be case-sensitive, but I'm
   not going to mess with it here.
3) nsHTMLEditor::IsSimpleModifiableNode.    I have no idea what this is trying to do.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff to fix the test issue

Comment 7

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

Comment on attachment 692137 [details] [diff] [review]
Interdiff to fix the test issue

Review of attachment 692137 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive compares instead?

In general, I'd prefer to get rid of the pseudo-ascii-case-insensitive code that we have rather than trying to migrate callers between the various ways of doing comparisons.

Do we really have a need for pseudo-ascii-case-insensitive anywhere?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive
> compares instead?

I did, then when I landed that patch it failed tests that expected a full-on Unicode compare for accesskeys.  So I looked at all consumers of eIgnoreCase, and found that there are three callsites who actually want Unicode case-insensitivity, so they have to stop using eIgnoreCase.  That's what the interdiff is about.  This is in addition to the first patch, which makes eIgnoreCase do ascii-case-insensitive stuff; I thought the comments in the interdiff made that clear....

> Do we really have a need for pseudo-ascii-case-insensitive anywhere?

I'm not sure what you're asking.  What's "pseudo-ascii-case-insensitive"?

Comment 9

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

(In reply to Boris Zbarsky (:bz) from comment #8)
> > I don't understand why we can't fix eIgnoreCase to do "real" ascii-case-insensitive
> > compares instead?
> 
> I did, then when I landed that patch it failed tests that expected a full-on
> Unicode compare for accesskeys.

Ah, makes sense.

> I'm not sure what you're asking.  What's "pseudo-ascii-case-insensitive"?

It's the crazy "ascii case insensitive plus special rules for kelvin and dotted I" that we apparently are using in some places.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It's the crazy "ascii case insensitive plus special rules for kelvin and dotted I" that
> we apparently are using in some places.

Ah.  Yeah, bug 820909.  I'm going to try to look at consumers of that too; just haven't had a chance to yet.  My gut feeling is likewise that no consumer actually wants that behavior.

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ffa4e1e64

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b9ffa4e1e64