Closed Bug 820902 Opened 12 years ago Closed 11 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files)

Right now it does nsCaseInsensitiveStringComparator, which is all wacky.
Assignee: nobody → bzbarsky
Whiteboard: [need review]
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f91a854410
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla20
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.
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.
> 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/mozilla-central/rev/7b9ffa4e1e64
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: