Closed Bug 35768 Opened 25 years ago Closed 22 years ago

[review]CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]

Categories

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

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: ess, Assigned: dbaron)

References

Details

(Keywords: css2, Whiteboard: [patch](py8ieh: need a thorough test case) (py8ieh:need test case which dynamically changes the xml:lang of a grandparent element))

Attachments

(11 files, 22 obsolete files)

Simple test-case: duplicates example in description. Expected result: "Foo, FOO, foo."
(deleted), text/html
Details
Slightly more valid version of previous test-case.
(deleted), text/html
Details
XHTML test case (also with xml:lang)
(deleted), text/xml
Details
testcase
(deleted), text/html
Details
HTML :lang test
(deleted), text/html
Details
XHTML xml:lang test w/ correct Javascript
(deleted), text/xml
Details
testcase, XML without namespaces
(deleted), text/xml
Details
patch updated according to bzbarsky's comments
(deleted), patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch to move to pointer-to-nsAutoString rather than nsAutoString
(deleted), patch
Details | Diff | Splinter Review
Deallocate the string too
(deleted), patch
sicking
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
New xml:lang testcase
(deleted), text/xml
Details
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m14) BuildID: 2000041210 HTML elements with their LANG attributes set are not styled according to appropriate :lang() stylesheet rules. This is probably the fault of bug 26237 (if I read correctly), but may be a fault in handling of CSS2. Reproducible: Always Steps to Reproduce: 1. Create style which depends upon :lang pseudo-class 2. Create HTML element with matching LANG attribute 3. View Actual Results: The style is not applied. Expected Results: The style should be applied to matching elements. As in: <html> <head> <style> span:lang(fr) { text-transform: uppercase; } </style> <body> <p>Foo, <span lang="fr">foo,</span> foo.</p> </body> </html>
Style System or Parser maybe. Trying Style System first.
Assignee: asadotzler → pierre
Component: Browser-General → Style System
QA Contact: jelwell → chrisd
*** Bug 14030 has been marked as a duplicate of this bug. ***
I STRONGLY suggest that this be marked "LATER" (i.e., fix in next version). We have not committed to implementing CSS2, and this is not the easiest of features to get right (it involves inheriting attributes down the DOM, and other interesting excursions). We have plenty of CSS1 bugs to keep us busy for a while yet. Furthermore, since we already have the dash match ("|=") attribute selector implemented, eager stylesheet authors can already, to some extent, get this functionality. BTW, we will need a thorough test case for this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: css2
OS: Windows NT → All
Priority: P3 → P4
Hardware: PC → All
Summary: CSS2 :lang pseudo-class won't match on LANG attributes → CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes
Whiteboard: LATER? (py8ieh: need a thorough test case)
Wholeheartedly approved. Marked LATER.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → LATER
Status: RESOLVED → VERIFIED
Whiteboard: LATER? (py8ieh: need a thorough test case) → (py8ieh: need a thorough test case)
Ok. We'll reopen this for 5.1...
Reopening and moving to Future. There probably exists duplicates of this bug.
Status: VERIFIED → REOPENED
Keywords: correctness
QA Contact: chrisd → py8ieh=bugzilla
Resolution: LATER → ---
Target Milestone: --- → Future
Summary: CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes → CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]
Blocks: Default-Quotes
*** Bug 61319 has been marked as a duplicate of this bug. ***
Nominating this bug for nsbeta1 on behalf of gerardok@netscape.com.
Keywords: nsbeta1
I'll append a first simple patch toward an implementation of the lang pseudo class. This allows the parser to handle it correctly. I hope this patch is uncontroversial. It changes the behavior of Mozilla a bit as far as to allow the rules with the :lang() pseudo-class to be used. It's just that the restrictions coming from the lang attribute are not checked. I don't think this situation is worse than what exists today. Anyhow, I'd like to implement this completely and already looked at the code. But there are some major decisions to be made first (see also http://bugzilla.mozilla.org/show_bug.cgi?id=24861 especially the comment by Daniel Glazman; I think the problem is similar). The problem here is that at all time it is necessary to know about the lang attribute for each tag. The initial language might even be defined by the HTML reply and/or the language selected by the user. Now it would be possible to always walk the tree from the current node to the root and check for the attributes but I guess this would be considered a problem by some people. But what is the alternative? Forward propagation of the lang attribute is costly as well. And what do you do with a DOM call changing/adding/removing the attribute? IMHO the tree-walking approach cannot be avoided without some major reorganizations of the code. To speed things up is is probably a good thing to handle the lang attribute special in the nsIContent class, always add it as an nsAtom* if set. This will reduce the lookup to pointer comparisons. I don't want to go ahead and do any of the work without some guidance. What do the owners of this code think is the right solution? I know that you declared this problem as "future" but to increase parallelism in the development this kind of guidance is necessary. Your priorities don't have to be shared by everybody and everybody should be fine to implement the missing pieces. I'd be happy to implement the simple, tree-walking approach now (which will have impacts almost only on those pages using the lang pseudo-class) and modify it later for a better solution.
Attached patch Implemenation of lang pseudo-class parsing (obsolete) (deleted) — — Splinter Review
Attached patch Corrected patch to implement :lang() parsing (obsolete) (deleted) — — Splinter Review
Please ignore the patch is attached on 07/05/01. It was missing and error check. The new patch has it and I'm using a build with the patch for 1 1/2 days now.
I believe we already "know" the language of each element, although I'm not sure how we do it. For example, right clicking on an element and choosing Properties will give you the language of the element, so the data is there somewhere. Hyatt? dbaron?
Ian Hickson wrote: > I believe we already "know" the language of each element I could see how. I looked at the information available in the CSS functions and there is no lang info anywhere. So I went on and finished my patch. I'll shortly attach it and in supercedes the patches I've provided so far. It should be a complete implementation with one defficiency which is as far as I can say not in the code I touched here. You can see the defficiency in the test input file I'll also attach (it's about changing the lang attribute of an element with the DOM functions). Creating new elements with DOM works as can be seen in the test case, too. Oh, the test case included in this bugzilla entry also works. As for the patch, it is I think the best you can do given the current set of information available in the nsIContent data. The parser stores the :lang() parameter as a string and the selector matching function tries to find an element with a lang attribute set and then performs a string comparison. Quite simple but potentially slow. There is, however, no negative effect on the performance for input files which Mozilla managed to handle so far. Only input with :lang() usage in the style sheet will use the new code. I don't know the standards of the Mozilla team but I would say this is good enough to include the code. For the future, I have some ideas but would really like to have some input first. I think that the lang attribute is important enough to be treated specially. Therefore a member GetLang() should be added to nsIContent. Alternative a data member mLang could be used. The language should be represented by an nsIAtom type object. The value will be automatically set if an attribute named "lang" is set using the SetAttribute members. When building the document structure the language of the document itself should be determined by the content-language specification in the HTTP reply. The patch I've provided would have to be changed only slightly: the nsAtomStringList type would be changed to nsAtomPairList (with mString becoming a nsIAtom* object), the CSS parser stores the language as an atom, and the selector matcher simply compares the nsIAtom from the CSS selector with the return value of data.GetLang(). No loops needed. While this solution is fast at display time it'll require more work when creating the document representation and also in the SetAttribute() functions. This should be acceptable especially since to solve the problem exposed in my test case (using the DOM function setAttribute to change the lang value) also requires the special treatment of such a call to initiated update of the display. Any comment welcome.
Attached patch complete implementation of :lang() (obsolete) (deleted) — — Splinter Review
Attached file more tests for :lang() handling (obsolete) (deleted) —
Attached file more tests for :lang() handling (obsolete) (deleted) —
Result of discussion in #mozilla about attachment 42159 [details]: the style hint for the 'quotes' property may need to be changed from REFLOW to FRAMECHANGE: http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsCSSPropList.h#188
I've compiled Mozilla with the proposed CSS_PROP(quotes, quotes, FRAMECHANGE) and have seen no difference. This doesn't fix the problem.
Hmm. The FRAMECHANGE change may or may not be necessary. However, what *is* necessary (should have thought of this earlier...) is changing CSSStyleSheetImpl::CheckRuleForAttributes at http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSStyleSheet.cpp#2037 to check for a lang selector and, if it exists, add the lang attribute (and xml:lang attribute, although this seems not to be namespace-aware -- it's just a performance hack) to the sheet's mRelevantAttributes. (Are there any other potentially relevant attributes?)
> However, what *is* necessary (should have thought of this > earlier...) is changing CSSStyleSheetImpl::CheckRuleForAttributes at >[...] I've looked at this (actually implemented it) and think this still cannot solve the problem. This code would help if the lang attribute of the element which has a CSS rule is changed. But the lang attribute is transitive to all the children (unless it's reset). So, in a situation like <p lang="de">some <q>quoted</q> text.</p> where there is a CSS rule with :lang() for <q> no the change would be recognized. The lang attribute is really special and might indeed need special attention. What makes things even stranger is that everything seems to work fine if I'm using colors instead of quotes. I've an updated test at http://www.cygnus.com/~drepper/quotes.html With the patch applied the DOM code changing the lang attribute of the DIV element with the ID "foo" has the effect that the color changes.
> This code would help if the lang attribute of the element which has a > CSS rule is changed. But the lang attribute is transitive to all the > children (unless it's reset). That shouldn't be a problem, since even with existing combinators we have to re-resolve style for all descendants when an attribute that affects style changes.
> That shouldn't be a problem, since even with existing combinators we have > to re-resolve style for all descendants when an attribute that affects style > changes. OK, then maybe you want to inspect what code I added: if (iter->mPseudoClassList) { // Search for the :lang() pseudo class. If it is there add // the lang attribute to the relevant attribute list. nsAtomStringList *runp = iter->mPseudoClassList; do { if (runp->mAtom == nsCSSAtoms::langPseudo) { // Found it. DependentAtomKey langKey(nsHTMLAtoms::lang); mInner->mRelevantAttributes.Put(&langKey, nsHTMLAtoms::lang); break; } } while ((runp = runp->mNext) != nsnull); } This is additional code in the for(iter=) loop in the function you mentioned. I've verified that the Put() function is actually called. Anything wrong with this?
Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is now actually being re-resolved, using the debugging tools in viewer? Or you could try another selector with "[lang]" as selector to work around the problem for now in case that code is wrong...
> Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is > now actually being re-resolved, using the debugging tools in viewer? You mean Debug|DOM Viewer? When I select this menu entry I get JavaScript error: line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMLocation.href]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: <unknown filename> :: oncommand :: line 0" data: no] Not when I press a button but when I select the menu entry. > Or you could try another selector with "[lang]" as selector to work around > the problem for now in case that code is wrong... If I replace :lang(de) with [lang="de"] nothing at all works. And I'd expect this since the lang attribute isn't set for the elements mentioned in the CSS.
> You mean Debug|DOM Viewer? no, mozilla-viewer.sh or viewer.exe > If I replace :lang(de) with [lang="de"] nothing at all works. Not replace, but add an additional rule so that the stylesheet is noted as having something that responds to the lang attribute.
> no, mozilla-viewer.sh or viewer.exe OK. I normally don't build with tests enabled. Have to recompile first. > Not replace, but add an additional rule so that the stylesheet is noted as > having something that responds to the lang attribute. Done that, no difference.
> Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is > now actually being re-resolved, using the debugging tools in viewer? I'm not at all experienced with the viewer debug tools but I think I tried every combination without getting any result. If you can suggest a specific procedure I'll do that.
Attached patch Updated patch after nsIContent change (obsolete) (deleted) — — Splinter Review
Please ignore the first added patch. I really shouldn't edit the patch file directly. But I have so many accumulated changes in those files that it's hard to regenerate the patch from scratch.
Attached patch *Corrected* updated patch (obsolete) (deleted) — — Splinter Review
Looks good (but that's not a formal review!). Do I take it you do not (yet) support the xml:lang attribute? I notice that as far as I can tell you explicitly look for the 'lang' attribute in the HTML namespace (right?). That seems wrong to me. You should look for the 'lang' attribute in NO namespace IF the element is an HTML element. The HTML spec doesn't say anything about a global 'lang' attribute in the HTML namespace. If I misread that code then my apologies.
> Looks good (but that's not a formal review!). Do I take it you do not (yet) > support the xml:lang attribute? You're right, xml_lang isn't supported. But it's not because of lack of trying. I couldn't figure out how. If you can point me in the right direction I'm happy to update my patch.
The xml:lang attribute SHOULD be the 'lang' attribute in the XML namespace. I'm not sure exactly how we do that. nisheeth/jst/heikki: could you help us here? How does one find the xml:lang attribute?
> The xml:lang attribute SHOULD be the 'lang' attribute in the XML namespace. I'm > not sure exactly how we do that. Unless I'm mistaken the GetAttr function in the HTML code will not allow using the kNameSpaceID_XML value (I think this is content/src/nsGenericHTMLElement.cpp:1876). And when looking through the data structures genereated for theXML file I couldn't find the xml:lang attribute altogether. I think I haven't yet attached a XML test case. Will do so after verifying it.
Attached file XHTML test case (also with xml:lang) (deleted) —
Have you checked if you can access xml:lang attribute in an XML document that has no declared namespaces? I know there is a bug regarding HTML/XHTML content: they do not support additional namespaces. Therefore, if you try to add xml:lang to an XHTML element it won't work. See bug 41983.
> Have you checked if you can access xml:lang attribute in an XML document that > has no declared namespaces? What I actually did is starting moz in gdb and looking at the code accessing the attributes to see how the xml:lang attributes are represented. The result: they are not there at all. In the XHTML test case I attached the only attributes visible are the xmlns attribute at the toplevel and the lang attributes in the preference test. No sign whatsoever of the xml:lang attributes. I might have look at the wrong place but it sure looks to me as if xml:lang isn't recognized as a valid attribute name and therefore not added to the internal representation.
There is a bug which makes them not appear for elements in the XHTML namespace. Look again, but on elements that have no namespace, e.g. in the following document: <foo xml:lang="en"/>
I've just created bug 98929 for a patch to implement Content-Language handling on document level. This implementation than can be used to (almost) finish the :lang() pseudo-class implementation (only xml:lang is missing). The patch is tested, see bug 98929 for an explanation how to configure Apache.
Keywords: patch
Attached patch Update after nsDocument change (obsolete) (deleted) — — Splinter Review
Now that nsDocument provides language information (bug 98929) I've updated the patch once more (attachment 49051 [details] [diff] [review]). This patch avoids converting from Unicode and does all conversion with Unicode chars. There is one more change: the language string returned from nsDocument can be a list. I think it only makes sense to use the first listed language and ignore the others when looking for a match.
This is not a full review - I thought the first 2 points below should be discussed before going further. 1) I haven't looked into it in detail but wouldn't have been simpler to add the language directly into the nsCSSSelector instead of putting it inside the mPseudoClassList? 2) I think you should not a match only on the first item in the 'Content- Language' list. You wrote in the patch that doing otherwise "might lead to not reproducible results". If you consider the example at http://www.w3.org/TR/REC-CSS2/selector.html#x42, the first pair of rules defines a precedence of HTML:lang(de) over HTML:lang(fr) in the case of a document that defines "fr,de" simply because the "de" rule comes after the "fr" rule. With the proposed patch, only the first rule applies, which is wrong - and it is even more wrong for documents that define "en,fr,de" since none of the 2 rules apply. 3) In nsAtomStringList::Equals() and SelectorMatches(), you should use EqualsIgnoreCase() to check the two strings. 4) I saw some NS_RELEASE where nsCOMPtr would be more appropriate.
Regarding point one, I'm not entirely sure I understand it, but :lang() is indeed just a pseudo-class, and rules like :lang(en):lang(de):lang(fr) ...and perfectly valid (if pointless) as are rules like: :not(:lang(x-klingon)):not(:lang(x-vulcan)):lang(x)
Severity: enhancement → normal
Status: REOPENED → ASSIGNED
Priority: P4 → P3
Target Milestone: Future → mozilla0.9.6
Blocks: 104166
I'll attach a patch that fixes #2, 3 and 4 above (#1 isn't such a good idea in case we need to implement more pseudo-classes that take a string as a parameter). I tested with a couple of testsuites and a simple testcase. Before loading the testcase, go to Prefs -> Navigator -> Languages and add "French" or "German" in addition to "English/US". Daniel or DBaron: please review Marc: please s/r
Attached patch patch 2.0 (obsolete) (deleted) — — Splinter Review
Attached file testcase (deleted) —
Comment on attachment 54201 [details] [diff] [review] patch 2.0 r=glazman I have only one concern : memory footprint... the pseudo-class list turned into a nsAtomStringList seems quite an expensive choice for a selector we don't use for the moment in our app stylesheets.
Attachment #54201 - Flags: review+
We should be fine regarding memory footprint: - The string is allocated only when needed, which means never except for :lang. - nsAtomStringList takes 3 pointers instead of 2 for nsAtomList, which is still ok even if we have hundreds of pseudo-classes. Note: I modified the patch a little bit in my local tree to optimize the use of language.Length().
Comment on attachment 54201 [details] [diff] [review] patch 2.0 >Index: mozilla/content/html/style/src/nsCSSStyleSheet.cpp >=================================================================== >RCS file: /m/pub/mozilla/content/html/style/src/nsCSSStyleSheet.cpp,v >retrieving revision 3.182 >diff -u -2 -r3.182 nsCSSStyleSheet.cpp >--- nsCSSStyleSheet.cpp 2001/09/29 08:25:58 3.182 >+++ nsCSSStyleSheet.cpp 2001/10/19 10:42:08 >@@ -2293,5 +2293,18 @@ > mInner->mRelevantAttributes.Put(&key, sel->mAttr); > } >+ if (iter->mPseudoClassList) { >+ // Search for the :lang() pseudo class. If it is there add >+ // the lang attribute to the relevant attribute list. >+ nsAtomStringList *runp = iter->mPseudoClassList; >+ do { >+ if (runp->mAtom == nsCSSAtoms::langPseudo) { >+ // Found it. >+ DependentAtomKey langKey(nsHTMLAtoms::lang); >+ mInner->mRelevantAttributes.Put(&langKey, nsHTMLAtoms::lang); >+ break; >+ } >+ } while ((runp = runp->mNext) != nsnull); > } >+ } > } /* fall-through */ > default: Why not a while-do without the extra if? (Well, OK, this may be better on some processors, so leave it if you want.) And also, the indentation seems odd (as in a few other places). Are there tabs here? This isn't a diff -w. >@@ -3331,5 +3344,5 @@ > if(PR_FALSE == mIsHTMLLink && > mHasAttributes && >- !(aContent->IsContentOfType(nsIContent::eHTML) || aContent->IsContentOfType(nsIContent::eXUL)) && >+ !(PR_TRUE == mIsHTMLContent || aContent->IsContentOfType(nsIContent::eXUL)) && > nsStyleUtil::IsSimpleXlink(aContent, mPresContext, &mLinkState)) { > mIsSimpleXLink = PR_TRUE; I really don't like the |PR_TRUE ==| -- it's especially dangerous for truth tests since occasionally booleans get set to a true value other than 1. >@@ -3450,4 +3463,30 @@ > > >+static PRBool DashMatchCompare(const nsString& comparedValue, const nsString& baseValue, const PRBool aCaseSensitive) { Parameters should be |const nsAString&| rather than |const nsString&| unless you have good reason to use things on |nsString| that aren't on |nsAString|, which you don't here. Also, below I'm going to point out a place where you need to be passing in an nsAString that isn't an nsString. >+ PRBool result; >+ PRUint32 baseLen = baseValue.Length(); >+ PRUint32 compLen = comparedValue.Length(); >+ if (baseLen > compLen) { >+ result = PR_FALSE; >+ } >+ else { >+ if (baseLen != compLen && >+ comparedValue.CharAt(baseLen) != PRUnichar('-')) { >+ // to match, the comparedValue must have a dash after the end of >+ // the baseValue's text (unless the baseValue and the comparedValue >+ // have the same text) >+ result = PR_FALSE; >+ } >+ else { >+ if (aCaseSensitive) >+ result = PRBool(NS_OK == Compare(Substring(comparedValue, 0, baseLen), baseValue, nsDefaultStringComparator())); >+ else >+ result = PRBool(NS_OK == Compare(Substring(comparedValue, 0, baseLen), baseValue, nsCaseInsensitiveStringComparator())); No need to cast to bool, and testing against NS_OK is wrong, it should be a test against 0 (which happens to be NS_OK, but Compare is like strcmp, it's not returning an nsresult). >+ } >+ } >+ return result; >+} >+ >+ > static PRBool SelectorMatches(SelectorMatchesData &data, > nsCSSSelector* aSelector, >@@ -3576,6 +3615,66 @@ > } > else if (nsCSSAtoms::langPseudo == pseudoClass->mAtom) { >- // XXX not yet implemented >- result = PR_FALSE; >+ NS_ASSERTION(nsnull != pseudoClass->mString, "null lang parameter"); >+ result = localFalse; >+ if (pseudoClass->mString) { >+ // We have to determine the language of the current element. Since >+ // this is currently no property and since the language is inherited >+ // from the parent we have to be prepared to look at all parent >+ // nodes. The language itself is encoded in the LANG attribute. >+ PRBool decided = PR_FALSE; >+ >+ nsCOMPtr<nsIContent> element = data.mContent; >+ while (element) { >+ PRInt32 attrCount = 0; >+ element->GetAttrCount(attrCount); >+ if (attrCount > 0) { >+ nsAutoString value; >+ nsresult attrState = element->GetAttr(kNameSpaceID_HTML, >+ nsHTMLAtoms::lang, value); >+ if (attrState == NS_CONTENT_ATTR_HAS_VALUE) { >+ // Compare the attribute string with the language string from >+ // the style sheet. >+ result = PRBool(localTrue == DashMatchCompare(value, *pseudoClass->mString, PR_FALSE)); >+ decided = PR_TRUE; >+ break; >+ } >+ } >+ nsIContent *parent; >+ element->GetParent(parent); >+ element = dont_AddRef(parent); >+ }; This is an extremely expensive loop for selector matching. Could you lazily cache the resulting language on the SelectorMatchesData (i.e., access it through a getter and store a state variable whether you've gotten it or not)? (We should start lazily caching other things on the SelectorMatchesData as well, so now is as good a time as any to start.) Also, there's a stray semicolon right at the end there. >+ >+ if (!decided) { >+ nsIDocument *doc; >+ if (NS_SUCCEEDED(data.mContent->GetDocument(doc))) { >+ // Try to get the language from the HTTP header or if this >+ // is missing as well from the preferences. >+ // The content language can be a comma-separated list of >+ // language codes. >+ nsAutoString language; >+ if (NS_SUCCEEDED(doc->GetContentLanguage(language)) && (language.Length() > 0)) { >+ if (nsnull != pseudoClass->mString) { >+ language.StripWhitespace(); >+ PRInt32 begin = 0; >+ PRInt32 end = 0; >+ while (begin < language.Length()) { >+ end = language.FindChar(PRUnichar(','), PR_FALSE, begin); >+ if (end == kNotFound) { >+ end = language.Length(); >+ } >+ nsAutoString httpLang; >+ language.Mid(httpLang, begin, end); >+ PRBool match = DashMatchCompare(*pseudoClass->mString, httpLang, PR_FALSE); Ugh, don't construct an nsAutoString and copy into it, just use |Substring(language, begin, end-begin)|. This requires that the parameter be a |const nsAString&| as I said above. >+ if (match) { >+ result = PRBool(localTrue == match); >+ break; >+ } >+ begin = end + 1; >+ } >+ } >+ } >+ } >+ } >+ } > } > else if (IsEventPseudo(pseudoClass->mAtom)) { >Index: mozilla/content/html/style/src/nsICSSStyleRule.h >=================================================================== >RCS file: /m/pub/mozilla/content/html/style/src/nsICSSStyleRule.h,v >retrieving revision 3.22 >diff -u -2 -r3.22 nsICSSStyleRule.h >--- nsICSSStyleRule.h 2001/09/25 01:31:18 3.22 >+++ nsICSSStyleRule.h 2001/10/19 10:41:47 >@@ -64,4 +64,17 @@ > }; > >+struct nsAtomStringList { >+public: >+ nsAtomStringList(nsIAtom* aAtom, const nsString *aString = nsnull); >+ nsAtomStringList(const nsString& aAtomValue, const nsString *aString = nsnull); >+ nsAtomStringList(const nsAtomStringList& aCopy); >+ ~nsAtomStringList(void); >+ PRBool Equals(const nsAtomStringList* aOther) const; >+ >+ nsIAtom* mAtom; >+ const nsString* mString; >+ nsAtomStringList* mNext; >+}; Bloat is a concern here, as Daniel said. Perhaps the language string should be an atom rather than a string? Also, mAtom should probably be an |nsCOMPtr<nsIAtom> mAtom| since it is an owning pointer, rather than dealing with manual NS_ADDREF/NS_RELEASE. There is a precedent for atomizing language strings -- see nsILanguageAtom in addition to nsIAtom. Using |nsString*| rather than |nsString&| and |nsString| is rather unconventional, although in this case we certainly don't want the bloat in all cases. Also, one could construct the linked list out of two classes, one derived from the other, and having an nsString member at the end being the only difference in the derived class, ensuring that you always had an instance of the derived class if the pseudo is lang, and casting. This would cause zero bloat increase except when there is a :lang pseudo.
> Why not a while-do without the extra if? Or, even better, a |for| loop.
Thanks for the review. I made the changes above except: - kept nsString: one of the methods is used. - did not convert mString to an atom: it would not bring much benefits if any, the language comparisons need to be case-insensitive, nsILanguageAtom is not an atom either. - did not add the casting: the total bloat is 8k (approx. 2000 strucs throughout the app), I chose legibility and maintainability over footprint but it can be debated - your call, really. Marc: s/r please.
Attached patch patch 2.1 (obsolete) (deleted) — — Splinter Review
> - kept nsString: one of the methods is used. Which method? Many of the methods have replacements. (See nsReadableUtils.h.)
The performance of this patch looks even worse than the previous one. SelectorMatchesData::GetLang should return an nsAFlatString&, not fill in an nsAWritableString&. I think you should also fix the other problems I mentioned.
- DashMatchCompare() needs CharAt(). - The casting would hurt the performance on the copy constructor. Result: - I just made a small change in my tree for GetLang() to return a pointer. Marc: s/r please.
Summary: CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT] → [review]CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]
Instead of CharAt you can use: nsAString::const_iterator iter; ... *comparedValue.BeginReading(iter).advance(baseLen) != PRUnichar('-') You should also address the other comments.
... or once I check in the patch on bug 104651 you could just use const |nsASingleFragmentString&| with the result of |Substring| on a known flat or single fragment string.
waterson/attinasi: please take a look at the performance problem below... > Instead of CharAt you can use: > nsAString::const_iterator iter; > *comparedValue.BeginReading(iter).advance(baseLen) != PRUnichar('-') The proposed patch creates a nsAutoString on the stack (guaranteed single fragment) and then calls CharAt(), both of fairly negligible CPU time. I'll leave it to super-reviewers to decide which solution is better but at first sight, the code above seems less legible and not really more efficient. > You should also address the other comments. I think that I included all the other changes except for the atomization of language strings and the overlapping structs because of the reasons listed in my previous comments.
I stand corrected: your solution with "Substring/const_iterator" is twice as fast as "Mid/CharAt". The times are quite variable but in average I get approximately 8 microseconds instead of 16. Thanks!
What is the overall performance impact on pages that do not make use of this feature? Also, there is some bloat here that is not going to help us make Mozilla smaller. Do we really need this now? As for the code, assuming that the base performance is unchanged, sr=attinasi, although it might be nice to move the logic out of SelectorMatches into an inline method to keep SelectorMatches more compact.
Creating an nsAutoString on the stack is *not* cheap when you do it many times. It used to be a significant percentage of time in SelectorMatches (above 10%, I think) was spent in the constructor and destructor of nsAutoString.
If you check this in, I'm going to have to go clean up the performance problems myself. I guess I'll just become the one who cleans up after you since you seem intent on checking this in.
My apologies if I did not make myself clear: the patch I intended to checkin addresses all your concerns about performance. The measurements I took after your comments yesterday showed that your solution resulted in an execution time of 8 microseconds instead of 16 microseconds and, of course, that's the one I had in my tree. Anyhow, it is not the right time to work on features like this. We'll reconsider later if the :lang() pseudo-class is worth 8k of bloat. MacIE5 implements this feature but not WinIE5 so the number of pages that currently use it is negligible. The 8k bloat seems to me unavoidable unless we accept a loss of performance. I'll attach an updated patch as a backup for when we come back to it.
Target Milestone: mozilla0.9.6 → mozilla1.0
Attached patch patch 2.2 (obsolete) (deleted) — — Splinter Review
There are two problems with that patch that I'd see at a quick glance: * GetLang could just return nsAFlatString&, not nsString*, and the IsEmpty check could be done by the caller. * GetLang should be caching the entire lang check, not just the first part of it.
>- did not convert mString to an atom: it would not bring much benefits if any, >the language comparisons need to be case-insensitive, nsILanguageAtom is not an >atom either. The langGroup is an atom (which by the way is used in the font engine to select adequate fonts). Could someone take heed to my request to have other string-like comparison APIs directly on nsIAtom? nsIAtom::StringEquals[IgnoreCase](aString), nsIAtom::StringEqualsIgnoreCase(anotherAtom), so that the ever changing string fu can be localized in one place. The usefulness of these APIs seems to be manifest in several contexts.
I don't think those APIs make sense -- I'd rather see a DependentAtomString function that returns a |const nsAFlatString&| (really a const nsDependentString) that is a dependent string on the atom, and then you can test (Compare(DependentAtomString(atom1), DependentAtomString(atom2), nsCaseInsensitiveStringComparator()) == 0).
Although this wouldn't be |nsresult|-like, so long as there is an easy way to do these string-fu operations which occur frequently and are often handled by people in suboptimal ways (e.g., string copies).
Keywords: mozilla1.0
David Baron told me the existence of this bug when he was reivewing bug 105199. Both bugs are handling "lang" html attribute, but probably for different purpose. Could the experts here take a look at that bug? We probably need a single patch to address both problems.
> We'll reconsider later if the :lang() pseudo-class is worth 8k of bloat. Please note that this blocks bug 16206, which is an HTML 4 compliance issue. While the Q element doesn't handle different languages (and nesting levels) intelligently, there's not much point in using it instead of using the presentational equivalent.
See also bug 115121.
Moving to mozilla1.1. Engineers are overloaded!
Target Milestone: mozilla1.0 → mozilla1.1
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling work post Mozilla1.0.
Priority: P3 → P1
Target Milestone: mozilla1.1 → Future
Blocks: 144853
cc'ing myself
Target Milestone: Future → mozilla1.2alpha
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
Priority: P1 → P2
Whiteboard: (py8ieh: need a thorough test case) → [patch](py8ieh: need a thorough test case)
Attached patch GetLang return value optimization (obsolete) (deleted) — — Splinter Review
Essentially patch 2.2, but the return value of GetLang is not tested for being an empty string twice. GetLang simply always returns a string reference.
The patch in attachment 90923 [details] [diff] [review] is basically patch 2.2 but with one little optimization dbaron mentioned: the return value of GetLang should not be tested twice for being empty. GetLang now always returns a nsString*. With regard to other optimizations mentioned: I don't think it is good to change GetLang to take a nsAutoString& parameter and put the value there. This would involve copying a string for no reason. If we don't have mLanguage in SelectorMatchesData we'd at least have to call element->GetAttr() for every GetLang call and copy the string. I hardly think this is an optimization. With regard to caching, is this really beneficial? Where do you suggest it to happen? We might be able to merge the DashMatchCompare functionality into GetLang (renaming it to IsLang or so) and cache the results for various lookups with different input strings. But this would make the SelectorMatchesData class even heavier. Is it worth it?
This is the previous patch, but merged to the current trunk (the diffs for that patch are against a really really old tree). I fixed a few style nits and other things, but there's still a few other things that I'd like to fix. Also, what's the deal with xml:lang?
Comment on attachment 91006 [details] [diff] [review] variant of previous patch merged to current trunk ignore this patch. There's something wrong with the selector matching code. (I should stop trying to do three things at once...)
Attachment #91006 - Attachment is obsolete: true
> Also, what's the deal with xml:lang? At the time when I wrote the patch the namespace handling was b0rken. Does it work correctly nowadays? I think the only necessary change is in GetLang where if (attrCount > 0) { nsAutoString value; nsresult attrState = element->GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::lang, value); if (attrState == NS_CONTENT_ATTR_HAS_VALUE) { mLanguage = value; break; } has to be replaced with something like if (attrCount > 0) { nsAutoString value; if ((IS_XML_DTD && element->GetAttr(kNameSpaceID_XML, nsHTMLAtoms::lang, value) == NS_CONTENT_ATTR_HAS_VALUE) || (element->GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::lang, value) == NS_CONTENT_ATTR_HAS_VALUE)) { mLanguage = value; break; } What I don't know is how IS_XML_DTD must look like.
Attached patch mostly working merged patch (obsolete) (deleted) — — Splinter Review
This is a mostly-working merged patch (modulo some issues with attachment 54202 [details]). I probably won't have a chance to look at this more for at least a few days, though, and I haven't looked through the whole thing yet closely. I did add the xml:lang support very quickly (and it seems to work based on attachment 48154 [details]). The attribute problems on HTML content should be fixed (thanks to sicking, I think).
Attached file HTML :lang test (deleted) —
More complete test based on 42159 and 42160.
Attachment #42159 - Attachment is obsolete: true
Attachment #42160 - Attachment is obsolete: true
I've attached a bit more complete test based on earlier tests of mine. With the patch applied the other tests in the attachments here seem to work. The just attached test also mostly works. The only problem is the second test with dynamically changing quotes (I am using the quotes patch from bug 24861). The change discussed in comment #20 does not fix the problem. Not that a very similar test which uses colors instead of quotes works just fine.
Blocks: 115121
Whiteboard: [patch](py8ieh: need a thorough test case) → [patch](py8ieh: need a thorough test case) (py8ieh:need test case which dynamically changes the xml:lang of a grandparent element)
Attached file XHTML xml:lang test (obsolete) (deleted) —
Attachment 91210 [details] contains the XHTML versio of the previous attachment. The results are not as good: - the same test as in the HTML test fails (expected, I guess) - the last test, adding a paragraph, fails in two ways + no coloring happens + the paragraph is not added as a paragraph. I.e., it's not on a separate line The latter might be my mistake. Let me know if it is. - when using setAttribute("xml:lang", la) instead of setAttribute("lang", la) the button-press tests fail. When using "lang" the coloring test succeeds. Note that this does contain setting the xml:lang of a grandfather node.
> The change discussed in comment #20 does not fix the problem. Since 'lang' is a special attribute with cascading effects, it may be that its impact has to become FRAMECHANGE too: http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#3504
> Since 'lang' is a special attribute with cascading effects, it may be that its > impact has to become FRAMECHANGE too: Yes, that seems to work: diff -u -r1.367 nsGenericHTMLElement.cpp --- content/html/content/src/nsGenericHTMLElement.cpp 4 Jul 2002 04:30:19 -0000 1.367 +++ content/html/content/src/nsGenericHTMLElement.cpp 19 Jul 2002 04:09:40 -0000 @@ -3509,7 +3509,7 @@ return PR_TRUE; } else if (nsHTMLAtoms::lang == aAttribute) { - aHint = NS_STYLE_HINT_REFLOW; // LANG attribute affects font selection + aHint = NS_STYLE_HINT_FRAMECHANGE; // LANG attribute affects font selection return PR_TRUE; } /* The only strange effect is that after pressing the butten the layout is a bit different. It seems as if an additional empty line got added. I get the same result with the XHTML test case. The only difference between the HTML and XHTML test case is now that adding a new paragraph simply appends the new text to the last line instead of adding a new line. And this only for XHTML.
> The only strange effect is that after pressing the butten the layout is a bit > different. Might be a layout bug then. > The only difference between the HTML and XHTML test case is now that adding a > new paragraph simply appends the new text to the last line instead of adding a > new line. And this only for XHTML. Looking at your script, the XHTML namespace seems to be missing: - var newp = document.createElement("p"); + var newp = document.createElementNS("http://www.w3.org/1999/xhtml", "p"); - newp.setAttribute("xml:lang", la); + newp.setAttribute("lang", la);
Fixing the Javascript code: use createElementNS.
Attachment #91210 - Attachment is obsolete: true
> Looking at your script, the XHTML namespace seems to be missing: You're right, thanks. I've updated the test, the result is in attachment 91921 [details]. The only issue remaining is the layout change after executing the DOM code for the first time. The patch at hand here seems to work fine. Can we get it reviewed and checked in?
Comment on attachment 91026 [details] [diff] [review] mostly working merged patch Is the following XXX comment still necessary? Looks like the patch is taking care of "xml:base" too, no? + // XXX What about xml:lang ? + PRInt32 attrCount = 0; + content->GetAttrCount(attrCount); + if (attrCount > 0) { + nsAutoString value; + nsresult attrState = content->GetAttr(kNameSpaceID_XML, + nsHTMLAtoms::lang, value); + if (attrState != NS_CONTENT_ATTR_HAS_VALUE && + content->IsContentOfType(nsIContent::eHTML)) { + attrState = content->GetAttr(kNameSpaceID_None, + nsHTMLAtoms::lang, value); + } Mind attaching a patch for the whole lot, i.e., with the other fixup?
s/xml:base/xml:lang/
Also, the comment here should probably tips at the :lang() pseudo too... + aHint = NS_STYLE_HINT_FRAMECHANGE; // LANG attribute affects font selection
This is exactly dbaron's patch without the comment questioning the xml:lang implementation.
Attachment #90923 - Attachment is obsolete: true
Patch misses the FRAMECHANGE bit, is it intentional?
> Patch misses the FRAMECHANGE bit, is it intentional? Yes, I don't know what exactly you want.
I mean the diff that you have in comment #93 (for which the comment needs to be updated to reference :lang too).
> I mean the diff that you have in comment #93 (for which the comment needs to > be updated to reference :lang too). Should this change really be part of this patch or should it be separately filed? If it should be here, how do you think the comment should change? return PR_TRUE; } else if (nsHTMLAtoms::lang == aAttribute) { // LANG attribute affects font selection and the :lang() pseudo class // in CSS can cause arbitrary changes aHint = NS_STYLE_HINT_FRAMECHANGE; return PR_TRUE; } /*
> Should this change really be part of this patch or should it be separately > filed? I think so, since it is needed for the patch to pass the testcases. It is the layout glitch that you mentioned that needs another bug (which might linger there as usual. In the meantime, let's have information wins over presentation. Plus it will be so rare to see a real website with a JS that flips the 'lang' anyway...). > If it should be here, how do you think the comment should change? Your proposed comment looks okay to me.
Again, dbaron's patch plus the comment change plus the patch described in comment 93 with updated comment in the sources.
Attachment #41289 - Attachment is obsolete: true
Attachment #41526 - Attachment is obsolete: true
Attachment #42158 - Attachment is obsolete: true
Attachment #46332 - Attachment is obsolete: true
Attachment #46333 - Attachment is obsolete: true
Attachment #48738 - Attachment is obsolete: true
Attachment #49051 - Attachment is obsolete: true
Attachment #49616 - Attachment is obsolete: true
Attachment #91922 - Attachment is obsolete: true
Comment on attachment 91928 [details] [diff] [review] Same as last patch + the change to set FRAMECHANGE hint for lang attribute r/sr=rbs. Nice job guys. This bug has been an exercise of determination and resilience. An edge over IE6 since my copy of it is failing miserably on the testcases.
Attachment #91928 - Flags: review+
I have applied the patch to check out the layout glitch... It is one of those flickers that sometimes happen in layout. It goes away at the slightest intention to resize the window -- meaning it is layout. All the valid attached testcases pass. The one in comment #80 has a bug. It claims that the lang of the document is 'de' when there is actually no lang there (e.g., the context menu doesn't display the 'Properties'). But continuing the test and clicking on the ':lang()' link shows that the rest works fine (again 'Properties' is a good indicator to see if the lang is set).
Comment on attachment 91928 [details] [diff] [review] Same as last patch + the change to set FRAMECHANGE hint for lang attribute >+++ mozilla/content/html/content/src/nsGenericHTMLElement.cpp 19 Jul 2002 08:02:01 -0000 >@@ -3509,7 +3509,9 @@ > return PR_TRUE; > } > else if (nsHTMLAtoms::lang == aAttribute) { Existing else after return is a non-sequitur -- fix while you're hacking here? /be >- aHint = NS_STYLE_HINT_REFLOW; // LANG attribute affects font selection >+ // LANG attribute affects font selection and the :lang() pseudo class >+ // in CSS can cause arbitrary changes, bug 35768 >+ aHint = NS_STYLE_HINT_FRAMECHANGE; > return PR_TRUE; > } > /*
Attached patch Same as last patch with useless 'else's removed (obsolete) (deleted) — — Splinter Review
Following Brendan's comment I've removed all the useless elses.
Attachment #91928 - Attachment is obsolete: true
Making the change hint for a lang attribute unconditionally be a framechange is incorrect. When attributes change, we will reresolve style if necessary, and compute the correct things that need to happen as a result of the style change. If AttributeAffectsStyle is implemented correctly, this should be unnecessary. (It probably isn't yet, for xml:lang.) There were also a few other changes I wanted to make to this patch.
That |#if 0|ed code about the class attribute being a framechange (in nsGenericHTMLElement.cpp) should just be removed completely. The mRelevantAttributes code should actually be sufficient for the AttributeAffectsStyle changes, since it's not namespace sensitive (although perhaps it's worth a comment that if it ever becomes namespace-sensitive, what we care about is the un-namespaced lang attribute on HTML-namespaced content and the lang attribute in the XML namespace.
> The mRelevantAttributes code should actually be sufficient It is not sufficient since the patch didn't pass all the tescases. I suspect the reason why it is not sufficient is because the frames that handle the quotes are created as generated frames and these have to be changed when the lang is changed. I know that it is possible to fix that, but as I noted, the dynamic change of 'lang' is so rare that I wouldn't give it such a high prioririty to the point of totally blocking the patch (it was mostly idealism vs. practicality). I can comment more about the peculiarity of xml:lang now that I have traced the code. But anyone tracing the code will see too.
Is the testcase that doesn't pass one that dynamically changes the 'quotes' property? It seems much more likely that the problem is one with dynamic changes to a specific property, rather than dynamic changes that affect certain types of selectors.
> Is the testcase that doesn't pass one that dynamically changes the 'quotes' > property? The test failing without the extra patch is the one qhich selects the quotes based on the lang attribute. The very similar test which selects a color based on the attribute succeeds.
Attachment #91998 - Flags: needs-work+
Comment on attachment 91998 [details] [diff] [review] Same as last patch with useless 'else's removed That means the lang attribute change in nsGenericHTMLElement.cpp should be replaced by a change to nsStyleQuotes::CalcDifference in nsStyleStruct.cpp (perhaps with a comment that it could be a reflow rather than a framechange if our quotes code worked differently).
> That means the lang attribute change in nsGenericHTMLElement.cpp should be > replaced by a change to nsStyleQuotes::CalcDifference in nsStyleStruct.cpp I've tried this patch after removing the nsGenericHTMLElement.cpp change. diff -u -r3.34 nsStyleStruct.cpp --- content/shared/src/nsStyleStruct.cpp 3 Jul 2002 17:14:31 -0000 3.34 +++ content/shared/src/nsStyleStruct.cpp 23 Jul 2002 07:18:42 -0000 @@ -1251,7 +1251,7 @@ PRUint32 ix = (mQuotesCount * 2); while (0 < ix--) { if (mQuotes[ix] != aOther.mQuotes[ix]) { - return NS_STYLE_HINT_REFLOW; + return NS_STYLE_HINT_FRAMECHANGE; } } I think this is what you went. The result isn't good: the test case does not work. Did you have something else in mind?
You need to change both occurrences of NS_STYLE_HINT_REFLOW in that function. (Which test case is it that doesn't work?)
> You need to change both occurrences of NS_STYLE_HINT_REFLOW in that function. Nope, no improvement. > (Which test case is it that doesn't work?) The one in attachment 91046 [details] and the one in 91921. In both cases the first test with buttons. The other tests work just like before the nsGenericHTMLElement.cpp change.
I don't see how it would make a difference in the testcase in question, but we also need to move the quotes section in nsStyleContext::CalcStyleDifference into the framechange section rather than the reflow section.
Incorporating dbaron's suggestions. This actually does produce the correct results.
I've implemented and tested what dbaron suggested. The patch in attachment 92599 [details] [diff] [review] removes the change to nsGenericHTMLElement infavor of changes to nsStyleStruct and nsStyleContext. I've also added appropriate comments. If this patch acceptable?
Comment on attachment 92599 [details] [diff] [review] patch for Change StyleStruct and StyleContext instead of GenericHTMLElemtn A bunch of nits, and two significant comments (first, moving the document language stuff out of SelectorMatches and into the RuleProcessorData; second, whether AtomStringList should hold raw PRUnichar* rather than nsString*). I *think* these should be it. (Yes, I realize I'm making this drag on forever...): > Index: mozilla/content/html/style/src/nsCSSParser.cpp > - nsIAtom* pseudo = NS_NewAtom(buffer); > + nsCOMPtr<nsIAtom> pseudo(dont_AddRef(NS_NewAtom(buffer))); > + Could be the equivalent form nsCOMPtr<nsIAtom> pseudo = do_GetAtom(buffer); and there's no need to introduce extra whitespace, I don't think. The parsing code is written in the style of the parser, which I can't stand much (since early returns are underused), but I won't complain since that's the style of the parser... > Index: mozilla/content/html/style/src/nsCSSStyleRule.cpp > - mAtom = NS_NewAtom(aAtomValue); > + mAtom = dont_AddRef(NS_NewAtom(aAtomValue)); Likewise, |mAtom = do_GetAtom(aAtomValue);|. > Index: mozilla/content/html/style/src/nsCSSStyleSheet.cpp > CSSStyleSheetImpl::GetEnabled(PRBool& aEnabled) const > { > - aEnabled = ((PR_TRUE == mDisabled) ? PR_FALSE : PR_TRUE); > + aEnabled = (mDisabled ? PR_FALSE : PR_TRUE); Ugh. How about |aEnabled = !mDisabled|? > @@ -2063,7 +2063,7 @@ > CSSStyleSheetImpl::SetEnabled(PRBool aEnabled) > { > PRBool oldState = mDisabled; > - mDisabled = ((PR_TRUE == aEnabled) ? PR_FALSE : PR_TRUE); > + mDisabled = (aEnabled ? PR_FALSE : PR_TRUE); Likewise. > else if (nsCSSAtoms::langPseudo == pseudoClass->mAtom) { > - // XXX not yet implemented > - result = PR_FALSE; > + NS_ASSERTION(nsnull != pseudoClass->mString, "null lang parameter"); > + result = localFalse; > + if (pseudoClass->mString) { > + // We have to determine the language of the current element. Since> + // this is currently no property and since the language is inherited > + // from the parent we have to be prepared to look at all parent > + // nodes. The language itself is encoded in the LANG attribute. > + const nsString* lang = data.GetLang(); > + if (!lang->IsEmpty()) { > + result = PRBool(localTrue == DashMatchCompare(*lang, *pseudoClass->mString, PR_FALSE)); > + } > + else { > + nsIDocument *doc; > + if (NS_SUCCEEDED(data.mContent->GetDocument(doc))) { > + // Try to get the language from the HTTP header or if this > + // is missing as well from the preferences. > + // The content language can be a comma-separated list of > + // language codes. > + nsAutoString language; > + if (NS_SUCCEEDED(doc->GetContentLanguage(language))) { This really needs to be in the RuleProcessorData (perhaps a second lazy getter function?). > + language.StripWhitespace(); > + PRInt32 begin = 0; > + PRInt32 end = 0; > + PRInt32 len = language.Length(); > + while (begin < len) { > + end = language.FindChar(PRUnichar(','), begin); > + if (end == kNotFound) { > + end = len; > + } > + PRBool match = DashMatchCompare(*pseudoClass->mString, Substring(language, begin, end-begin), PR_FALSE); > + if (match) { > + result = PRBool(localTrue == match); This could be simplified to result = localTrue; > + break; > + } > + begin = end + 1; > Index: mozilla/content/html/style/src/nsICSSStyleRule.h > +struct nsAtomStringList { It's worth a comment above this that the class is optimized for the case that there is no string and it should not be used in cases where the string is usually present. However, that might not be necessary given the following... Also, is there a reason we want this class to store an |nsString*| rather than a raw |PRUnichar*| (which requires one allocation rather than two)? I certainly would have used the latter if I were writing this myself.
Two notes: * we should add a comment pointing to http://www.w3.org/TR/xhtml1/#C_7 which says that the xml:lang takes precedence over lang on an HTML-namespace element. * xml:lang="" should probably be taken to "un-define" the language as far as the document is concerned. We should double-check that we do that.
Never mind about the content language stuff needing to be in the rule processor data -- that won't speed it up significantly, since there's a good bit of matching to do even once we have it.
This is the previous patch, with the changes I mentioned in comment 124, excluding the one I mentioned in comment 126, plus a fix for a leaked reference to an nsIDocument in SelectorMatches and an additional comment change I had lying around in my tree.
Attachment #54201 - Attachment is obsolete: true
Attachment #55546 - Attachment is obsolete: true
Attachment #56308 - Attachment is obsolete: true
Attachment #91026 - Attachment is obsolete: true
Attachment #91998 - Attachment is obsolete: true
Attachment #92599 - Attachment is obsolete: true
nsCSSParser::ParseLangSelector: + if (! GetToken(aErrorCode, PR_FALSE)) { // premature eof + REPORT_UNEXPECTED_EOF(NS_LITERAL_STRING("argument to :lang selector")); + aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR; + return; + } Should that really be a PR_FALSE in GetToken()? Is whitespace not allowed after the '('? (We need that CSS3 grammar ;) ). Especially since we do "ExpectSymbol(aErrorCode, ')', PR_TRUE)" later... The code in nsAtomStringList::Equals has lots of comparisons to "nsnull" that should, imo, be simple boolean tests (eg |if (aOther)| instead of |if (nsnull != aOther)|. I guess that's the style of the code in that file, but... (David's call as module owner here) nsAtomStringList is checking the "rest of the list" before checking the existence and equality of mString and aOther.mString. Was this done for a reason? If so, add a comment explaining that reason so some well-meaning soul does not "fix" it. (David mentioned this in comment 124, I see). It feels like there should be a comment where you declare mPseudoClassList as an nsAtomStringList that explains what the "string" part of nsAtomStringList means for a pseudo-class.... (that this is used for "function pseudo-classes and the string is whatever was in the parens). Would it be worthwhile to make mLanguage in RuleProcessorData an nsString* (or even nsAutoString*) instead of an nsAutoString and to allocate it on the first call to GetLang()? The null-ness (or not) of the pointer could then be used in place of mIsLanguageValid, methinks. It should make the common-case RuleProcessorData struct smaller and should not really make the uncommon case that much slower, imo... (just a suggestion; feel free to tell me I'm on crack here). + result = localTrue == DashMatchCompare(*lang, + nsDependentString(pseudoClass->mString), PR_FALSE); ... + nsDependentString langString(pseudoClass->mString); ... + if (DashMatchCompare(langString, + Substring(language, begin, end-begin), + PR_FALSE)) { One of those two DashMatchCompare calls has the string args in the wrong order. It would be clear which is wrong if DashMatchCompare had argument names that made it clear which argument comes from the selector and which argument comes from the content node or if it had a comment on the topic. As far as I can tell, the DashMatchCompare against the Substring() should have Substring() and langString reversed. If there is no content-language on the document and there is no lang attr on anything, we will treat :lang() as a no-op with this code, no? That is, both a:lang(foo) and a:lang(bar) will match an anchor. Further, both :lang(foo) and :not(:lang(foo)) will match everything... Is this really what we want? > + result = PRBool(localTrue == DashMatchCompare(value, attr->mValue, isCaseSensitive)); Do you really need the PRBool() part of that? Please adjust the comments in nsStyleContext.cpp that currently say: 568 // FRAMECHANGE Structs: Display, XUL, Content, UserInterface 637 // REFLOW Structs: Font, Margin, Padding, Border, List, Position, Text, TextReset, 638 // Visibility, Quotes, Table, TableBorder since you are making Quotes a FRAMECHANGE struct. The rest looks good.
http://www.w3.org/TR/css3-selectors/#grammar is clear that whitespace is allowed in functional pseudos.
> If there is no content-language on the document and there is no lang attr on > anything, we will treat :lang() as a no-op with this code, no? That is, both > a:lang(foo) and a:lang(bar) will match an anchor. Further, both :lang(foo) and > :not(:lang(foo)) will match everything... Is this really what we want? I don't see what makes you think this. I did make the change that makes ":lang()" never match, though (and thus ":not(:lang())" always matches). I'll attach a new patch that should address bzbarsky's comments.
I didn't bother with the nsAutoString pointer -- I don't think the RuleProcessorData is constructed enough that we have to worry about a single nsAutoString, although I could be wrong. Also see my previous comments.
Attachment #94955 - Attachment is obsolete: true
Comment on attachment 95149 [details] [diff] [review] patch updated according to bzbarsky's comments > + result = attributeSubstring.Equals(aSelectorValue, > + nsCaseInsensitiveStringComparator()); Odd indent there (one char off). > I don't see what makes you think this. I had missed the |+ result = localFalse;| line at the beginning of the lang-matching code.. ;) sr=bzbarsky
Attachment #95149 - Flags: superreview+
Checked in to trunk, 2002-08-14 05:34 PDT. Thanks to all who contributed, especially Ulrich Drepper, for all the work that's gone into this patch. (That odd indentation was intentional, to avoid crossing the 80 character boundary.)
Status: NEW → RESOLVED
Closed: 25 years ago → 22 years ago
Resolution: --- → FIXED
This patch goes on top of the previous one -- it moves to bzbarsky's suggestion in comment 128 of using an |nsAutoString*| in RuleProcessorData for better performance in the case when there are no :lang selectors. I just checked this patch in because tinderbox showed a roughly 1.5% page load performance regression on two separate tinderboxes after this patch landed, and this is the only thing I can think of that might have caused it.
I also removed the extraneous |SetLength(0)| which peterv pointed out.
Well, either the two cycles where the page load numbers were higher were a random blip that's a bit higher than most, or that really did make a difference, because the numbers look like they're back where they were. I really need to remember never to underestimate the amount of time that nsAutoString's constructor takes!
Attached patch Deallocate the string too (deleted) — — Splinter Review
This just adds a "delete mLanguage;" to the destructor of RuleProcessorData
Comment on attachment 95285 [details] [diff] [review] Deallocate the string too Yeah, thanks. :-) rs=dbaron
Attachment #95285 - Flags: superreview+
Woo hoo! Finally resolved after a year. Thanks for the last changes. Could somebody now at least comment on the patch in bug 24861? It's one of the few internationalization related bugs which are outstanding. And another step close to supporting CSS2.
I'm not sure about this: <p lang="de">bla<span lang="en">?</span></p> and the style: *:lang(de){color:blue} ..should the "?" be blue or black (default)...?
It should be blue, since color inherits and you've specified no rules on the lang="en" content to override that color.
In all the testcases, the xml:lang tests involved inheriting down from another tag, such as a div being tagged xml:lang whatever and all b tags within it inherit a certain style. However, the simple case of styling (for example) all P's of xml:lang whatever to a given style does not work. Reopening bug, will be attaching testcase to illustrate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file New xml:lang testcase (deleted) —
In this test, all P's of lang(fr) are red. P's tagged with only the "lang" attribute get the style. P's tagged with only the "xml:lang" attribute do not. Since "lang" is deprecated in XHTML 1.1, this becomes a problem.
if you send that file as text/xml it works.
Comment on attachment 95593 [details] New xml:lang testcase xml:lang, and namespaces in general, are not supported in the HTML parser. Changing mimetype of testcase to "text/xml", which is one of the valid XML MIME types.
Attachment #95593 - Attachment mime type: text/html → text/xml
Testcase works fine when changed to text/xml. Marking fixed again.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
verified
realy verifying
Status: RESOLVED → VERIFIED
*** Bug 41978 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: