Closed Bug 93371 Opened 23 years ago Closed 23 years ago

WRMB: Treat classes case insensitively in quirks mode

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: ian, Assigned: pierre)

References

(
URL
)

Details

(Keywords: compat, css1, topembed, Whiteboard: checked in on trunk and 0.9.2 branch [DIGBug] [PDT+])

Attachments

(7 files)

Patch: converts classes in HTML and CSS to upper case in quirks mode
3.13 KB, patch
Details | Diff | Splinter Review
testcase 1.0
628 bytes, text/html
Details
Patch: converts classes to upper case in quirks mode, including on nodes that were dynamically added
3.29 KB, patch
Details | Diff | Splinter Review
test case illustrating selectorText case change
1.42 KB, text/html
Details
patch v2
19.62 KB, patch
Details | Diff | Splinter Review
Patch v3 (for 0.9.4 branch)
16.81 KB, patch
asa
: approval+
Details | Diff | Splinter Review
Patch v3 (for tip of the tree)
17.02 KB, patch
Details | Diff | Splinter Review 
We've had feedback from many high profile sites that they would appreciate it
if we implemented a quirk to treat classes case insensitively in quirks mode.
Uh hum... You rubber-stamped it WontFix in bug 35522.  See my comments from 
[2000-04-14 20:42] and the following ones.

I fundamentally totally completely definitely absolutely disagree with such a
modification. Sorry this is early morning here and I can't think of more
adverbs to say "NO!".
URL: http://www.hixie.ch/tests/adhoc/css/s...
Status: NEW → ASSIGNED
Keywords: compat, css1, patch, topembed
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Whiteboard: [Hixie-PF] WONTFIX ? -- non standards compliant quirk RFE
Target Milestone: --- → mozilla0.9.4
I don't have an opinion on whether we should or should not do this in quirks
mode. I shall let you all argue it with Eric Meyer.

Netscape people: see WRMB 7380.

Pierre: My patch only tries to 'fix' this for classes, but for these I got
around your concerns in bug 35522 by not changing to a case insensitive compare
but instead uppercasing the classes in the CSS parser and HTML attribute code.
This is actually what we used to do, peterl changed it back in 1999, and my code
is basically his old code with a NavQuirks check around it.

dbaron: I know very little about strings in Mozilla, and have some concerns
about the code, could you take a look and point out what I did wrong? 
(Regardless of whether this gets in -- I'd like to learn from my mistakes if
possible.) Thanks!

Testcases:
   http://www.hixie.ch/tests/adhoc/css/selectors/class/001.html
   http://www.hixie.ch/tests/adhoc/css/selectors/class/compatibility/001.html
You are going to screw the dom if you canonize the strings.  I'll attach a 
testcase which, I guess, will fail if you check in this patch.
Attached file testcase 1.0 — 

    
    

    
  
See bug 49004.

    
    

    
  
I am bemused that hixie is pushing for this one when at the same time he gives a 
hard time to MathML authors that have expressed their desire to have a hack for 
.xml/.xhtml documents. Or is it because "many high profile sites" are involved 
here, and principles don't count anymore, go figure.

    
    

    
  
Pierre: Your test case works with my patch, fwiw.

rbs: Like I said, I have no opinion on what we do in quirks mode. I am not
pushing for this, nor pushing against it. In fact it was me who added "WONTFIX?"
to this bug's status whiteboard. Note also for the record that I have said that
I would be quite happy with a "magic comment" fix for the issue to which you
refer, so it is somewhat unfair to say that I give "a hard time to MathML 
authors that have expressed their desire to have a hack for .xml/.xhtml 
documents". All other proposed solutions have been shown to have flaws.

Also, quirks are about getting old pages to render right to increase Mozilla's
market acceptance in order to promote standards. We shouldn't have to implement 
quirks for new areas that will not significantly impact Mozilla's marketshare.

    
    

    
  
Since my name got invoked, I'll add my thirty pieces of silver.  Let me provide
a perspective from a pseudo-outsider point of view first.  As someone who's
spent the last few years promoting standards, and also a guy who's been "in the
trenches" of Web design, I think it makes a lot of sense to make use this patch,
thus making class and ID values case-insensitive in Quirks mode only.  That's
not a contradiction in stances.  Quirks mode, so far as I can tell, is intended
to allow Mozilla to have "bugwards compatibility" with old browsers and legacy
authoring practices.  Well, treating class and ID values as case-insensitive is
a legacy practice.  If you aren't interested in legacy support then there's no
reason to have Quirks mode at all.

How does this help with standards evangelism?  It's much easier to teach someone
the right way if you've already helped them escape trouble for inadvertantly
doing it the wrong way.  The saying "you can catch more flies with honey than
with vinegar" comes to mind here.  One can say, "Okay, we've all done this bad
thing, and browsers will continue to handle it for a while, but we need to get
onto the case-sensitive bandwagon if we want to move forward and use more
advanced stuff."  It's much harder to say, "Yes, your pages are broken and your
boss is screaming at you because of this little mistake that everyone else made
that Netscape doesn't handle.  So fix your case now and it won't be a problem."  

Which one is more likely to work?  Especially when they can say, "Well, it works
in IE, what's wrong with Netscape?  Can't you people code ANYthing right?" 
Trust me-- authors don't read specs.  They assume that anything a browser will
let them do is "okay."  To tell them otherwise induces denial, anger, and often
rejection of an opposing view.  It isn't pretty.

Now, from a more Mozilla-centric point of view, I would not support this change
if it significantly broke behaviors in strict mode, nor would I if it somehow
toppled a row of bigger dominoes.  If it can be confined to Quirks mode, and it
doesn't open bigger cans of worms, then I'm all for it.  So far, I don't see any
fallen dominoes, except perhaps a loss of some purity-- but by design, Quirks
mode isn't exactly the realm of the pure.
So can anyone point to a major practical problem which would be created by
having case-insensitive handling of class and ID values while in Quirks mode,
but using to case-sensitive handling in strict mode?

    
    

    
  
(mid-air collision: re-submitting...)

I am in favor of implementing that quirk for the reasons listed by Ian but I 
recalled that canonizing strings would be a no-no.

Daniel: why are you so much against the quirk?  I don't think that people who are 
aware of the case-sensitivity would rely on it to differentiate classes.  The 
thing that I do want to avoid is to break pages that use the same class names 
everywhere (which is what I wanted to show in my testcase).


    
    

    
  
hixie: Clarification taken. I turn to you as a _man of standards_ and I wished
somebody else was dirtying his hands with this bug.

    
    

    
  
Eric, I think we are on an agreement here.

Now, does someone have any idea why canonizing strings could be bad?  Or help me 
prove it doesn't have any side-effects?


    
    

    
  
It does have one sideeffect, which is that asking for class, as in:

   myElement.className

...would, with this patch, always return an upper case class, so if someone 
used that to do case-sensitive comparisons, things would break.

In practice I think that is much (much MUCH) less frequent than having CSS with
classes in one case and HTML with classes in another. (In fact, the only place
I have ever seen anyone do this is in my own test cases.)

    
    

    
  
If you are going to check this in with the DOM side effects, wouldn't it be
better to at least normalize to lower case, since the custom seems to be that
CSS classes are lower case?

    
    

    
  
The patch is fine with me if Daniel doesn't come up with very strong reasons, and 
if Johnny accepts to sign on this too.

Henri: I prefer to normalize to uppercase.  If some people (hopefully a very few 
of them) experience problems with their scripts, they are more likely to notice 
that "checkBox" is turned to "CHECKBOX" rather than "checkbox".

Daniel: please comment.
Johnny: please review and sign on this dom breakage.


    
    

    
  
About the patch itself: once you have the document, you can QI for 
nsIHTMLDocument then GetDTDMode(nsDTDMode& aMode) and check for 
"eDTDMode_strict!= aMode".


    
    

    
  
Sigh.  After having helped to beat out the perpetual "why don't you show 
tooltips for ALT" debate, I'm a little disturbed to see this change, and the 
fact that it's being argued for (at least by some) on the grounds that "it's 
Quirks mode, that's for backwards compatibility by definition, we can shove 
whatever we want in there to accomodate poor design left over from old 
browsers."  I agree that we should have Quirks mode, but I think it should 
generally be reserved for real page-destroying backwards compatibility issues: 
otherwise, people will simply come to assume that standards mode is this weird 
thing that happens when you include a doctype, and simply write quirky markup 
forever.  Quirks mode *has* to induce some breakage, or people will never 
bother to fix things.  I'm somewhat suspicious of a request coming this late in 
the game-if this was really causing horrible page breakage, I'd think the issue 
would have come up earlier for debate (which I see that it has in bug 35522)-
but I'm not going to carry on a long argument.  I just hope everyone pushing 
this has read the previous discussion in bug 35522 and thought carefully about 
whether we *really* need to provide another incentive for authors to use Quirks 
mode forever.

    
    

    
  
Unfortunately I think that as our technology becomes adopted into browsers for a 
more mainstream public, we'll have more pressure to implement the most visible 
quirks.  I'm taking bets for the support of the Layer tag in 6.5 and the IE 
absolute positioning rules in 7.0.


    
    

    
  
waterson, any idea of the font(s) that eXceed triggers at every window open and
every window close? That's pretty intriguing.

    
    

    
  
wrong bug -- sorry!

    
    

    
  
Pierre: is a QI slower or faster than walking up two pointers? I did think of
that when writing the patch but I seem to remember scc saying something about
wanting to avoid QIs if at all possible...

cc'ing scc for some string and performance love on the patch. scc: Given 
Pierre's comments above (and the fact that if the QI fails then we know we
are not in quirks mode) what is the best solution? Also, please make sure I
did the string stuff the right way, I'm afraid I did it mainly through cut
and paste and so am not very confident that I did it all right. Thanks!

choess: As you know, I am generally against adding quirks. Apparently class 
case sensitivity issues are breaking a lot of sites that are important to Gecko 
embedders, though, such as computingcentral.com, gohip.com, stoneage.com, 
indepedenttravel.com, indepedenttravel.com, moneycentral.com, homeadvisor.com, 
marketguide.com, miniessentials.com, indepedenttravel.com, etc.

    
    

    
  
Uppercasing the value of class attributes in HTML for quirks mode is not a big
deal for the DOM, it might however be a big deal for the editor for
roundtrippability of documents. Cc:ing beppe for her input on this issue.

As far as the patch goes, I don't feel strongly either way (QI vs. Ian's
approach), I doubt anyone can show one approach being noticeably faster/slower
than the other. I kinda like Ian's approach since it's independent of the type
of document the HTML element is found in, but this doesn't really matter for now
since we're only ever in quirks mode if we're really parsing good ol' HTML.

One more thing to think of is what happens when a class attribute is set on an
element that is dynamically created and not yet in the document? With the
current patch we won't get a document so we won't know if we're in quirks mode
or not, this could be fixed by getting the document like this:

  nsCOMPtr<nsINodeInfo> ni;
  aContent->GetNodeInfo(*getter_AddRefs(ni));
  ni->GetDocument(*getter_AddRefs(doc));

This is guaranteed to get you the document even if this element isn't part of
the document yet (assuming the document still exists).
As Ian points out here, element.className will be uppercased as well with this
patch, but since this is for quirks mode I don't think it's a big deal,
element.className didn't even work in pre 6.0 versions of Netscape, it might
have worked in early versions of IE tho, that I don't know w/o testing and I
don't have IE 4.0 here. Bob Clary, thoughts about uppercasing element.className?

    
    

    
  
jst: shouldn't new nodes always have a document? I thougth that was what the 
DOM specs said...

    
    

    
  
I have some problems with changing the case of the class name at all, whether to
all upper case or lower case. The following seems hard to justify to a script
author.

elm.style.className = 'foo';
...
if (elm.style.className == 'FOO')
...

I think that such a change would break any existing script that compared class
names. While it would possibly not affect many sites, since they do not use
className via script, it would certainly break almost all of my scripts and
perhaps most of the existing widely distributed script libraries.

I understand the hashing issue is more complicated than the automatic adjusting
of the case and am not proposing that the decision be revisited.

I think that if the only feasible solution to support case insensitive class
names in quirks mode is to convert all class names to upper case, then we
*should not do it*.

Now for all of you who have beaten up on Hixie about this. 

I am one of those who proposed this case insensitive treatment of class name and
id attributes in order to improve the performance of Mozilla on existing web
pages.  If it is technically feasible, and won't break existing scripts and is
limited to quirks mode then I still think it is a good idea. 

Personally, I think a Quirks mode that emulates Netscape Navigator 4 is a bad
idea and would much prefer to not have to try to live with that browser's bugs
any longer than I have to. A Quirks mode that recognizes the general bad coding
habits of web authors and does a *reasonable* job of displaying their content
however was, and still is, a good idea. 

If you believe that this issue of case insensitive class names in quirks mode is
such a bad idea, why don't you try to eliminate the other quirks? Or even quirks
mode in general? If all you want is a reference implementation of a Standards
Compliant browser that will never be used by more than a few thousand or even
tens of thousands of people, then that is the way to go about it.

Either way, get off of Hixie's case about it.


    
    

    
  
#3 (which hixie clarified) "why make much fuss against some and be lenient viz. 
others."

    
    

    
  
Hey guys, stop working during the week-end !-)

I am against this change for the following reasons :

1. in my mind quirks were added to emulate ol'time-browsers errors mostly caused
   by (a) the browsers' war (b) holes in W3C standards. What Ian proposes seems
   to me a **MAJOR** incentive for web providers to keep making mistakes in
   their web site. This is about fixing an error made by web authors, not adding
   a quirk.
2. case-sensitivity of class names has NEVER been a hole in a standard. Classes
   have always been case-sensitive, whatever is the conformance of the document
   to standards. I am ready to raise an issue in CSS WG if Microsoft has
   products accepting case insensitive classes.
3. there is not a single HTML+CSS editor on the market accepting case
   insensitivity of classes
4. implementing CSS in Composer, I certainly don't want to deal with such a
   possibility
5. I guess that the next step is case-insensitivity of ID's ?
6. people wanting to convert document from very old style + case-insensitive
   classes to DTD conformance will have big surprises...

IMHO, all this issue is an evangelism problem (although it might be hard to
evangelise computinccentral.com, it belongs to microsoft ;-) and not at all
an implementation problem. I am still strongly opposed to this modification.

    
    

    
  
> jst: shouldn't new nodes always have a document? I thougth that was what the 
> DOM specs said...

See bug 52732.



    
    

    
  
Thanks Johnny for dragging my name in on this :o)

From the editor perspective: as long as we retain roundtrip integrity (i.e. do 
not alter the users initial document whatsoever under the hood) and as long as 
the output of the editor remains compliant (in regards to new documents), then 
promote non-standards support as much as your little heart desires.

Whatever happened to the mantra: size, speed and standards?

    
    

    
  
beppe: you understand that we will have to introduce a difference in CSS editing
for "no DTD at all at composer's launch time" and "conformant to a HTML dtd" ?
For the time being, if you take Ian's test case [1] and open in in browser, choose
Edit Page and save it immediately, we add a doctype...
Hmmmm....

[1] http://www.hixie.ch/tests/adhoc/css/selectors/class/compatibility/001.html 

    
    

    
  
yes, we do add the required doctype, so agents know which dtd to validate 
against. if the required doctype is not present, the user would be unable to 
validate the document. both situations make it necessary to add the basic 
requirement of the doctype.

    
    

    
  
Right! So if we edit a dtd-less tag-soup containing case-insensitive classes in
Composer, the stylesheet might be broken because we change dtd-conformance of
the document and therefore switch to case-sensitive classes.
That is imho a strong argument against adding case-insensitivity of classes to
quirks mode.

    
    

    
  
First, I'm with Bob Clary: don't beat up on Hixie for this one.  Bob and I,
while working on evangelism cases for Netscape, discovered that a lot of sites
were mixing case on their class names (less often on IDs, because IDs aren't
often used).  We ran some tests through the W3C HTML validator and it complained
about defining case-insensitive-matching ID values.  I complained on
www-validator and was shown that HTML defines class and ID names to be
case-sensitive, but that it also forbids case-insensitive-matching ID (and name)
values.

So things are already confused, as ID values are not purely case-sensitive in
HTML documents anyway.  Every browser Microsoft has ever published is
case-insensitive in this area, so far as I know.

To respond to Daniel's points:

1. What Ian (actually Bob and I) proposes is to make it easier for us to
evangelize the Right Way.  It's far simpler to publish a note which says "these
are case-sensitive, but legacy bugs allowed them to be insensitive, so that's
what we're doing in quirks mode in order to not break your scripts/pages.  For
the future, especially in XML, be case-sensitive.  And please go back to fix
your old pages if you get a chance."  Otherwise we leave them 'broken' and
encourage both bad authoring and abandonment of anything based on Gecko for
being "too picky."
2. By all means raise the issue in the CSS WG.  Try the HTML WG while you're at
it.  The best you can hope for is to get MS to shove the case-insensitivity into
their quirks mode.  Why shouldn't we get there ahead of them?
3. Thank goodness.  Over time, then, we can expect this issue to fade into
obscurity.  During the intervening years, however, we probably ought to take
steps to avoid pissing off authors.
4. Can't help you there.
5. Um, that's part of what we're proposing for quirks mode already.
6. People converting from legacy authoring to strict authoring are already in
for a huge raft of nasty shocks and surprises.  This one will be comparatively
simple (and even understandable) when compared to the others.

As for the issues discussed in bug 35522, I've already said that I'm in favor of
this as long as it doesn't actively break other parts of Mozilla.  If it does,
then we need to consider a different course of action.  I didn't see anything
which would break due to 35522, but then I'm not a programmer, so maybe I missed
something.  I too would prefer to avoid forcing all-uppercase DOM values, even
in quirks mode-- is there no other way to achieve case-insensitivity in quirks mode?

    
    

    
  
Isn't it so that
* Mozilla stores ASCII-only strings as ASCII
* Most legacy sites use ASCII only in the class name anyway, because trying to
use full Unicode would be just asking for trouble no matter what the spec says
?
If the above is true, could the case insensitivity be implemented at ASCII
string comparison time in the style system by using fast bitwise comparisons
that ignore the ASCII case bit? What would the performance implications be?

That's not to say I like this quirk. I'm not quite sure whether it is good in
the long run.

    
    

    
  
> 2. By all means raise the issue in the CSS WG.  Try the HTML WG while you're 
> at it.  The best you can hope for is to get MS to shove the case-
> insensitivity into their quirks mode.  Why shouldn't we get there ahead of 
> them?

Microsoft Windows IE 6 Preview already has this quirk. In standard mode they 
are case sensitive, in quirks mode they are case insensitive.

See: 
   http://www.hixie.ch/tests/adhoc/css/selectors/class/compatibility/001.html
...which triggers their (and our) quirks mode, and
   http://www.hixie.ch/tests/adhoc/css/selectors/class/001.html
...which triggers their (and our) standard mode.

    
    

    
  
Great News! I did some further testing and found that in fact my code doesn't
change the literal value of the class, only the internal parsed value! So my
test passes this test fine:
   http://www.hixie.ch/tests/adhoc/dom/html/attributes/compatibility/001.html

Given that, it would seem the only person now against adding this quirk is 
Daniel, as this resolves all the other concerns I've heard (e.g. editor round
tripping and MSDN getting broken).

    
    

    
  
Great! I'm all for this change then, I sympathize with Daniel here, but we have
enough problems that need to be evangelized, and this is easy to fix in our code
in a way that makes us "compatible" with IE 6. With my above comments about the
patch addressed, sr=jst

    
    

    
  
Make us compatible with IE6 ? Geeeeezzzz.... This sentence is frightening. IE6
is not a standard, what these web providers do in their erroneous style rules is
not a standard. We have standards on the table and they explicitely say that
classes are case-sensitive. Period, imho.

"Mozilla is an open-source web browser, designed for standards-compliance,
performance and portability" ; excerpt from from [1]

Adding this quirk is not only a matter of technique ; it will impact the market
by the legitimization of incorrect ways of writing stylesheets.

Any quirk "officialized" by the two major browsers becomes a 'de facto'
standard. We already saw that in the past. Sorry, but I still like ONLY 'de
jure' standards and think this quirk is a mistake.

[1] http://www.mozilla.org/mozorg.html


    
    

    
  
Daniel, you know better, compliance with IE 6 is not the reason for doing this
(there was a reason for the quotes around compliance in my last comment),
compliance with older browsers is the reason for doing this, and the fact that
IE 6 has the same quirk suggests that there's enough existing content out there
that relies on this quirk for Microsoft to bother coding this quirk. If I'd
haveto choose between supporting standards or supporting existing content, all
in quirks mode, I'd haveto go with supporting existing content, that's what
quirks mode is all about. Especially in this case since the fix doesn't really
break anything but edge cases (i.e. if you use class names that differ only by
character case).

If there's one thing that gecko does badly today (from my point of view) it's
rendering (and working with) old content, whatever we can do about that w/o
messing things up too badly is IMO well worth doing. There's only so much our
evangelism team can do, Bob n' his team deserves a break every now n' then :-)

    
    

    
  
Ian just told me on IRC that 4.x accept case-insensitive classes and I have
checked that it is the case :-( That changes completely my point of view because
there is on this problem an historical pre-v5s background. So I change my opinion
and accept this quirk, even if I would prefer evangelism.

(no I don't accept because _netscape_4 was doing that error ; I would accept too
 if _ie_4 was guilty)


    
    

    
  
I shall work on an updated patch tomorrow.
Keywords: patch

    
    

    
  
Hixie, to answer your question wether nodes always should have a document in the
DOM, the answer is yes, node.ownerDocument should always point to the document
that the node is in, or the document that created the node if the node is not in
a document. But that's not exactly how mozilla works, especially that's not how
nsIContent::GetDocument() works, GetDocument() is an internal method that only
gives you the document if the node is in the document, if the node is an element
then nsIDOMNode::GetOwnerDocument() gives you the document even if the node is a
new node that hasn't yet been put in the document (again, assuming the document
is still around). If the node is not an element then GetOwnerDocument() works
exactly like GetDocument() for now. This is not perfect wrt compliance with the
DOM spec, but it's about as close as we'll get w/o bending over backwards to
make this work 100%...

    
    

    
  
String fu looks okay.

    
    

    
  
How will this affect style sheet sharing in a case where two documents use the
same external style sheet but one doc activates the quirks mode while the other
activates the standards mode?

Does Mozilla share style sheet between docs at all? If not, can sharing be
expected to be implemented in the future?

    
    

    
  
As soon as my trunk build works, I'll be able to check that with this test case:
   http://www.hixie.ch/tests/adhoc/css/selectors/class/compatibility/002.html

    
    

    
  
Yes, my patch passes that test.

    
    

    
  
Ok, that last patch takes jst's comments into account.

Looking for r=, sr=, and check-in into trunk please. :-)
Keywords: patch
QA Contact: ian → bclary
Whiteboard: [Hixie-PF] WONTFIX ? -- non standards compliant quirk RFE → [fix in hand]

    
    

    
  
Ian : to tell the truth, I don't really like adding PresShell & PresContext
knowledge to ParseClasses. I would have preferred a way of checking the
compatibility mode staying at document's level, perhaps like Pierre proposed
above.

    
    

    
  
Marc, Waterson? We need super-review on this, especially the doc->GetShellAt(0).
See my comments from [2001-08-03 15:27].  After all, the CSSParser belongs to the 
CSSLoader which belongs to the HTMLDocument.


    
    

    
  
Pierre, what is the relationship between DTDMode and nsCompatibility? Are they
really the same thing? I do not think that they are (at least not conceptually,
even if they are currently the same in practice). 

I'm willing to SR this with or wiothout the coupling to the presContext for the
compatibility mode (something tells me you have little choice, other than to
pass the mode into the CSSParser) - sr=attinasi  - let me know if you need me to
check it in for you too, once you have the r= (Pierre?)


    
    

    
  
The compatibility is defined like this:
    SetCompatibilityMode(((eDTDMode_strict== mDTDMode) ? 
                         eCompatibility_Standard : 
                         eCompatibility_NavQuirks));
We had long discussions 1 or 2 years ago that finally led to this definition.

r=pierre for either solution, that's why I asked for another opinion. It is the 
question of relationship between objects that made me raise my eyebrow: I 
understand the link between a parser and a document, but not between a parser and 
a presContext (or more precisely between the parser and the presContext of the 
1st presShell of the document).  Will we want someday to parse quirky documents 
just to get their content?  Maybe.


    
    

    
  
I forgot to add that either way, Ian deserves kudos for having looked further 
into the problem than I and other folks did a long time ago, and proved that 
there was no side-effect in implementing the quirk like this.


    
    

    
  
What is the status on this bug? I see that Pierre has done the r= -- who is 
doing the sr=?

When will this get checked in?
Summary: Treat classes case insensitively in quirks mode → WRMB: Treat classes case insensitively in quirks mode

    
    

    
  
Pierre r'd it, attinasi (that's me) already sr'd it - I guess we were waiting to
see if Ian could check it in himself or needs a checkin-buddy. Ian, let me know
if you want me to comit the patch for you.

    
    

    
  
He'll need a check-in buddy.

P.S. Don't close this bug till it's also checked in on the branch.

    
    

    
  
Why?  He has cvs access now.

But while we're on the topic of Ian, anyone have anything personal or
embarrassing to share about him?

    
    

    
  
Since when? I had to check something in for him very recently.

    
    

    
  
Checked in on trunk. Marc, could you check it in on the 0.9.2 branch? I don't
have a branch set up and I won't have time to set it up before I leave. Thanks!
Whiteboard: [fix in hand] → checked in on trunk, awaiting branch checkin

    
    

    
  
I'm happy to check this into the branch for you - is it ready to go now? (sorry,
I still do not understand when it is OK to commit to the branch...)

    
    

    
  
Checked into branch. I had to manually apply the change to nsHTMLAttributes
because of conflicts due (apparently) to the changes to convert GetUnicode() to
get(), but I triple checked and I think I got it right.

/cvsroot/mozilla/content/html/style/src/nsCSSParser.cpp,v  <--  nsCSSParser.cpp
new revision: 3.142.10.2; previous revision: 3.142.10.1

/cvsroot/mozilla/content/html/style/src/nsHTMLAttributes.cpp,v  <-- 
nsHTMLAttributes.cpp
new revision: 3.49.4.1; previous revision: 3.49

Closing now as FIXED
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
  
Whiteboard: checked in on trunk, awaiting branch checkin → checked in on trunk and 0.9.2 branch

    
    

    
  
this fix actually causes an unwanted side effect with regard to the
document.styleSheets[i].rules[j].selectorText values which is not acceptable for
script authors.

Re Opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
since Hixie is out for a few weeks, I am reassigning this to the default owner. 
Assignee: ian → dbaron
Status: REOPENED → NEW
QA Contact: bclary → ian

    
    

    
  
Correct: 'selectorText' is screwed and 'cssText' too.  I'm afraid it is one of 
the testcases that would work only if we changed the string match functions 
instead of the strings themselves, which is why bug 35522 ended up WontFix.

Now the questions are:  
1) Is it really unacceptable to authors if we change the case of 'selectorText' 
in quirks mode?  After all, CSS-DOM doesn't exist in the browsers that are 
responsible for the quirky documents.
2) Considering the question above, what is the most important in quirks mode?  
Make 'selectorText' work correctly, or treat classes case-insensitively?

Considering the current testcase, I still vote to keep the code as it is now.  It 
is very unlikely that an application will expect an exact match between a 
selectorText and a className (or more exactly '.' + className).  Personally, I 
would be more concerned by the round-tripability of 'cssText' but I can't come up 
with an example that would justify to back-out the fix.  Maybe the Editor folks 
can help me there?  Or those of you who are copied on the (Netscape-internal) 
Bugscape #8973?


    
    

    
  
I'll look if there is a cheaper solution than what I described in bug 35522 on 
[2000-04-14 20:42].  Maybe we can turn the strings to uppercase when calculating 
the hash key?

Assignee: dbaron → pierre

    
    

    
  
Not sure if we have any problem with that in Composer because the first thing
Composer does is adding a doctype... That means we are never editing in Quirks
mode, right ?

If I am not right, then getting a false selectorText will be very problematic
for me.

    
    

    
  
About Composer:
- When there is a doctype, we respect it and don't modify it.
- When there is no doctype, we add a transitional doctype.  It means that the 
document is edited in quirks mode.
- Documents that are created with Composer get a transitional doctype. 
- We may need a pref or a setting in Composer to create documents in strict mode.

I'll look into it.  If there is no satisfying solution, it will be a difficult 
call between leaving or removing this fix.  Both sides are topEmbed.


    
    

    
  
All docs created by Composer use a quirky doctype. Bug 92474 was about the real
issue. Sadly, it was marked as a duplicate of a bug that is less focused and
isn't likely to get fixed.

    
    

    
  
Thanks Pierre. I need in JS to rely on selectorText for the steps 3 and 4
described in document [1]... and adding css support to composer is very
important for aol mail and aol hometown. So, yes, there is a choice to make
here.

[1] http://www.mozilla.org/editor/adding-css-to-editor.html

    
    

    
  
To reenforce Bob Clary's comments, losing case sensitivity when searching for
document.styleSheets[i].rules[j].selectorText is unacceptable for developers
using DHTML.  From DIG's point of view, it would be a serious flaw in Gecko, if
this side effect is allowed to go forward into the product.


Whiteboard: checked in on trunk and 0.9.2 branch → checked in on trunk and 0.9.2 branch [DIGBug]

    
    

    
  
Pierre, what's so hard about making the class selector matching code case
insensitive in quirks mode (I haven't looked, just asking)?

    
    

    
  
From bug 35522"

------- Additional Comments From Pierre Saslawsky 2000-04-14 20:42 -------

I'm giving up on that one. It requires 3 kinds of changes.
- The first one is fine: for the IDs to be case-insensitive, we have to do in 
SelectorMatches() the same kind of thing we already do for bug 24390 (see the 
comments in the function).
- The second one is fine too: for the classes to be case-insensitive, we should 
pass in HasClass() a boolean to specify if the string compare done at a lower 
lever should be case-sensitive or not, depending on whether we are in quirks
mode and inside a HTML content. It requires changing several interfaces but it's
still ok.
- The last change is more tricky. In RuleHash::EnumerateAllRules(), we keep hash 
tables for IDs and classes. These ones can't easily be made case-sensitive 
depending on the content and the compatibility mode. We can alwyas find a 
solution but it would be a bit hacky and hurt the performance, so I prefer to 
give up on that quirk. We may reconsider if other sites show the same problem. 
This one was fixed yesterday by the webmaster. Marking as LATER.

-------------------

Dunno how relevant this still is, but that third change was why it got latered
and later wontfixed.

    
    

    
  
Classes and IDs are stored in hash tables, which means that you don't compare the 
strings but their hash keys and once the hash keys are calculated, you can no 
longer do the comparison depending on the context. I deemed the problem tricky 
and costly in bug 35522 last year because (IIRC) my goal was to fall back on the 
quirk only when necessary. It means that a quirky document with an element of 
class "Test" would have tried to match "Test" before falling back to "TEST". On a 
second thought, it may not be necessary: a solution where the strings are 
converted to uppercase before calcuting the hash key could be acceptable too.  
I'll look into it Real Soon Now (today, I promise). It's the kind of problem 
where finding a solution is easy; making sure it doesn't break anything takes a 
bit more time.


    
    

    
  
Pierre, couldn't that be solved by using different hash key
generation/comparison code depending on the quirk? I.e. when you create the
PLHashTable you pass it different function ptr's, or when you create the
nsHashKey you create different ones depending on the quirk (i.e. you create your
own case insensitive nsStringKey). Again, just curious...

    
    

    
  
Yes, that's what I described in my last comments: go for a simpler step #3 than 
what I was thinking of in my comments from bug 35522, and just turn the strings 
to uppercase before calculating the hashKey (that's in AppendRuleToTable() and in 
EnumerateAllRules()).  Step #1 and #2 still need to be implemented.  It's easy 
and I already did so.  I quickly looked at changing the behavior of the hash 
tables when creating them but it might not be as convenient, or maybe result in 
more allocations.


    
    

    
  
*** Bug 97527 has been marked as a duplicate of this bug. ***

    
    

    
  
I have a solution which I think comes at a minimal cost in terms of performance.

I'm seeking reviewers who enjoy looking into the smallest details
jst / dbaron / daniel / waterson ?


Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2
        — 
          — Splinter Review
      

    


  

    
    

    
  
The hash function in the quirks case looks very expensive -- constructing a
single nsAutoString is quite expensive (thanks to the design of the obsolete
string classes).  I would think it would be better to convert the class to
lowercase before putting it in the hashtable.

    
    

    
  
Then again, maybe the performance of classes in quirks mode isn't a major issue,
since it doesn't affect the chrome, which is where we need these optimizations
the most.  I still think it's worth profiling some pages that use CSS classes a
bit and do trigger quirks mode.

    
    

    
  
Also, I thought we still wanted to leave IDs case-sensitive, even in quirks mode.

    
    

    
  
Correct: I did it that way to avoid hurting the chrome.  We still use much more 
case sensitive operations than case insensitive, and not just because of the 
elements in strict mode in the chrome: we have the tags and the attributes too.

Here are some very rough numbers:

                OPEN A BLANK WINDOW               LOAD CNN.COM
               sensitive  non-sensitive       sensitive  non-sensitive
ctor             24400        100                71600     800
::HashCode()     25900        200                68500    1100
::Equals()        8800          0                20800     300


About the IDs, I remember that the issue was more sensitive than for classes (I 
remember someone saying ironically "Making classes case-insensitive?! Why not the 
IDs while we're at it?").  The problem is that authors did make the same mistake 
as for classes.  The original problem, bug 35522, was reported because IDs did 
not match.


    
    

    
  
According to HTML 4.01 (and, I presume, xhtml 1.1) IDs are case-sensitive, but
they cannot be a case-insensitive match for any other ID value or any NAME
value.  I don't know how that affects things in quirks, though.  It seems to me
that whatever NN4.x and IE5.x/Win did, we ought to get as close as we can to
doing the same.  I'm pretty sure that means making IDs case-insensitive as well
(in quirks mode only, natch).

    
    

    
  
IE and Nav4 are case insensitive for IDs: that's why bug 35522 was filed.


    
    

    
  
pierre : I wonder why you add case-sensitivity switch to XULattributes. I
understand that you have to change the interface but why do you care about
case-sensitivity of classes in nsXULAttributes.cpp ? I think you should just
ignore aCaseSensitive there and make the usual case-sensitive comparison.
Otherwise, r=glazman

    
    

    
  
I agree with glazman.  This quirk should not be supported by XUL elements.


    
    

    
  
hyatt: when HTML forms use XUL widgets, will we need this quirk in XUL elements?


    
    

    
  
I see that this has a target milestone of 0.9.4.  Calendar is going to need this 
fix on the 0.9.2 branch, because that is what is being rolled into compuserve.

    
    

    
  
Set milestone to mozilla0.9.5 added nsbranch keyword
Keywords: nsbranch
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  
pierre: nope.

    
    

    
  
It is possible that the checkin made in the name of this bug (by Ian Hickson) 
broke one of the XBL test cases (see bug #98295).


    
    

    
  
Yes, indeed. Ian's checkin caused a regression.  The class name in the CSS is 
not being matched by the DIV element in the XBL, even though the cases match 
perfectly.  My guess is there's a code path whereby something is not getting 
uppercased.

I am assuming that the new patch will fix this problem, since it relies on 
using EqualsIgnoreCase instead of actually hacking the values of the parsed 
class list?


    
    

    
  
If this is not going to be fixed for 0.9.4, then I am going to recommend that 
Ian's checkin be backed out of 0.9.4.  

    
    

    
  
Kevin: why did you set the milestone to 0.9.5?  This bug really should be fixed 
on 0.9.2 and 0.9.4 branches.

hyatt & others: this bug is waiting for reviews and approval.  The only one so 
far is "r=glazman without the quirk in XUL elements".

Whiteboard: checked in on trunk and 0.9.2 branch [DIGBug]

    
    

    
  
*** Bug 98295 has been marked as a duplicate of this bug. ***

    
    

    
  
Ian's checkin has caused a major regression for any elements created 
dynamically using the DOM or cloned using the DOM.  The problem is that he has 
hacked into the class parsing routine, which is called as soon as a class 
attribute is set.

The problem arises when a DOM node is cloned and put into a new document or 
when the document is changed.  In the XBL case, a DOM node from a strict 
document (an XML document) is being cloned and inserted into a quirks document 
(an HTML document).  Because the class list is cloned and not reparsed, the 
uppercase fixup never happens, and the node in the HTML doc ends up failing to 
match the style rules in the doc.

Furthermore, going from quirks to strict is even worse, since the class list of 
the element was actually mutated to uppercase.  When placed into the strict 
document, the list will still be uppercased and will not match the original 
specified case on the element.

IMO this patch should be backed out of 0.9.4, given the severity of the 
regression.


Whiteboard: checked in on trunk and 0.9.2 branch [DIGBug]

    
    

    
  
Pierre, does your new patch address these issues? i.e., does it avoid mutating 
the class list?  If so, then I'll review ASAP and we can get it into 0.9.4.


    
    

    
  
Yes: the proposed patch passes all the testcases so far, including the one at bug 
98295.


    
    

    
  
The class & ID lists are not modified.  During style resolution and selector 
matching, we compare the atoms' pointers (in strict mode) or the CRC of the 
atoms' uppercased strings (in quirks mode).


    
    

    
  
if we can get this in on the 0.9.4 branch before mozilla releases
then lets try.  this could use a good milestone testing verfication...

otherwise the option is to take the fix on the 0.9.4 branch after
the mozilla release in some additional work netscape will do there
that leads up to the next netscape release.

after we get this landed somewhere and we get a chance for many
people to pound on it to make sure the fix is right and has no
side effects then we can look at the risk/benefit of it landing
on older branches.

this has the nsbranch keyword so its on the right path to 
get put in the right branches in the plan I outlined above.




    
    

    
  
sr=hyatt.  This is a must-have for the branch.



    
    

    
  
Wouldn't AtomKey_base::Equals() be much more efficient if we'd simply do a case
insensitive string compare in stead of doing two string copies just to calculate
the hashcodes for the two strings (which is the wrong thing to do too, string
hashcode equality doesn't guarantee string equality, right?).
AtomKey_base::Equals() should checks if a given key is the same key as itself,
not if the two keys generate the same hashcode, unless I missed something here.
(AtomKey_base::Equals() also has a else-after-return in it, btw)

Wouldn't this be what we want?

PRBool AtomKey_base::Equals(const nsHashKey* aKey) const
{
  if (mCaseSensitive) {
    return PRBool (((AtomKey_base*)aKey)->mAtom == mAtom);
  }

#ifdef DEBUG_HASH
  DebugHashCount(PR_FALSE);
#endif
  const PRUnichar *myStr = nsnull;
  mAtom->GetUnicode(&myStr);

  nsIAtom* theirAtom = ((AtomKey_base*)aKey)->mAtom; 

  const PRUnichar *theirStr = nsnull;
  theirAtom->GetUnicode(&theirStr);

  return nsCRT::strcasecmp(myStr, theirStr) == 0;
}

No string copying, no possibility of memory allocations.

    
    

    
  
What jst said (also, a CRC is not a general TLA for hashcode).

/be

    
    

    
  
I did something like that at first but it did not work.  I'm the one who may have 
missed something because your version works fine.  I'll use it, of course.


    
    

    
  
brendan: AtomKey_base calculates the hash code using the same function that 
ensures that atoms are unique.


    
    

    
  
I'm going to attach a Patch v3 that does not implement the quirk for XUL elements 
and uses jst's code for AtomKey_base::Equals().

Patch v3 is equivalent to Patch v2 after reviews and corrections from dbaron/
glazman/hyatt/jst.  The version numbers of the files in Patch v3 correspond to 
the 0.9.4 branch.  I added "PDT" to the status whiteboard to get approval for 
0.9.4 and 0.9.2.

Patch v3 will be updated to the tip of the tree very soon and checked in as 
r=glazman/sr=hyatt

Whiteboard: checked in on trunk and 0.9.2 branch [DIGBug] → checked in on trunk and 0.9.2 branch [DIGBug] PDT

    
    

    
  


  

    
    

    
  
Sorry to be too late. But I didn't think of it earlier :-(
Wouldn't that be much cleaner/leaner to just add a method to the effect
  nsIAtom::StringEquals(PRBool aCaseSensitive)
in xpcom/ds/nsAtomTable ?
That will have save string copy and cut this patch significantly.

    
    

    
  
- nsIAtom::StringEquals(PRBool aCaseSensitive)
+ nsIAtom::StringEquals(nsAString& aString, PRBool aCaseSensitive)

    
    

    
  
rbs: no need to.  nsIAtom->GetUnicode(const PRUnichar **aResult) doesn't copy 
strings, it simply returns the pointer.


    
    

    
  
Patch v3 was checked in onto the tip.
Waiting for PDT and drivers' decision for 0.9.4/0.9.2...


    
    

    
  
Waiting anxiously for a fix on the branch :)

    
    

    
  
We want this on the 0.9.4 branch.  I added PDT+ and nsbranch+.  Thanks!
Keywords: nsbranch → nsbranch+
Whiteboard: checked in on trunk and 0.9.2 branch [DIGBug] PDT → checked in on trunk and 0.9.2 branch [DIGBug] [PDT+]

    
    

    
  
Comment on attachment 48498 [details] [diff] [review]
Patch v3 (for 0.9.4 branch)

a=asa on behalf of drivers for checkin to 0.9.4 branch

        Attachment #48498 -
        Flags: approval+

    
  
Blocks: 99225

    
    

    
  
why wasn't it checked into 0.9.4 (although, I haven't checked the bonsai)

    
    

    
  
Sorry, I forgot about it...  I'll check it in today.


    
    

    
  
Fix checked into MOZILLA_0_9_4_BRANCH.  Per the latest guidelines, I will not 
check anything into 0.9.2, not even back out Ian's code.

Marked fixed.

Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
is this already on the trunk (I want to make sure we don't lose it)?

    
    

    
  
The patch was checked into the trunk on 2001-09-06 and into MOZILLA_0_9_4_BRANCH 
on 2001-09-14.


    
    

    
  
FWIW, a profile I just took of loading http://www.google.com/ showed 0.25% of
the time spent within AtomKey_base::HashCode.

    
    

    
  
dbaron: hopefully the percentage is at its highest in a page like this one where 
almost all the elements are styled.


    
    

    
  
I have verified that the testcase 1.0 in the attachments section and the
testcases Ian outlined:
Testcases:
http://www.hixie.ch/tests/adhoc/css/selectors/class/001.html
http://www.hixie.ch/tests/adhoc/css/selectors/class/compatibility/001.html

appear to function properly (shows correct colors) using a Mac OS X and Win32
build 2001-10-05-094.

Need add'l verification on the trunk and Linux branch 094 branch build.  Ian,
will you be able to verify?  If not, pls change the QA contact back to me for
the rest of the verification.  Thanks.

    
    

    
  
Have not heard back.   Gerardo, can you get this bug verified on the 094 branch
for linux and mark the bug with the vtrunk keyword so it can be verified on the
trunk later?  If you can get to verifying on the trunk, that'd be great too. 
Thanks.

    
    

    
  
Verified on 2001-10-17-04-0.9.4 build on Linux.
Keywords: vtrunk

    
    

    
  
*** Bug 88594 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 127467 has been marked as a duplicate of this bug. ***

    
    

    
  
verified this on 
linux : 2002-04029-07 trunk build
macOS 9.1 : 2002-04-12-03 trunk build 
win2000 : 2002-03-01 trunk build

marking bug verified.

Status: RESOLVED → VERIFIED
Keywords: vtrunk

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: