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

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla20
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Right now it does nsCaseInsensitiveStringComparator, which is all wacky.
Testcase at http://people.mozilla.org/~jdaggett/tests/casesensitivity-classid.html
Created attachment 691461 [details] [diff] [review]
nsAttrValue::Equals should do ASCII-case-insensitive compares when it's doing case-insensitive compares.
Attachment #691461 - Flags: review?(jonas)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #691461 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f91a854410
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla20

Comment 4

4 years ago
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
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.
Created attachment 692137 [details] [diff] [review]
Interdiff to fix the test issue
Attachment #692137 - Flags: review?(jonas)
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?
> 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"?
(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.
Attachment #692137 - Flags: review?(jonas) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9ffa4e1e64
https://hg.mozilla.org/mozilla-central/rev/7b9ffa4e1e64
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.