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: