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."
155 bytes, text/html
Details
Slightly more valid version of previous test-case.
288 bytes, text/html
Details
XHTML test case (also with xml:lang)
3.14 KB, text/xml
Details
testcase
425 bytes, text/html
Details
HTML :lang test
2.75 KB, text/html
Details
XHTML xml:lang test w/ correct Javascript
2.96 KB, text/xml
Details
testcase, XML without namespaces
401 bytes, text/xml
Details
patch updated according to bzbarsky's comments
32.67 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch to move to pointer-to-nsAutoString rather than nsAutoString
2.54 KB, patch
Details | Diff | Splinter Review
Deallocate the string too
857 bytes, patch
sicking
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
New xml:lang testcase
591 bytes, 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.

  

  



    
    

    
  


  

  

  



    
    

    
  


  

  

  



    
    

    
  
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 file
          
        more tests for :lang() handling (obsolete)
        — 
      

    


  

  

  



    
    

    
  

      
      
      
      

        Attached file
          
        more tests for :lang() handling (obsolete)
        — 
      

    


  

  

  



    
    

    
  
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.



  

  



    
    

    
  


  

  

  



    
    

    
  
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)
        — 
          — 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.

  

  



    
    

    
  


  

  

  



    
    

    
  
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)
        — 
          — 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)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  

  

  



    
    

    
  
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)
        — 
          — 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)
        — 
          — 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

    
    

    
  
cc'ing myself

  

  



    
    

    
  
There is a test case at http://www.richinstyle.com/test/language/lang2.php.

  

  


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)

    
    

    
  


  
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)
        — 
          — 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
        — 
      

    


  
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)
        — 
      

    


  

  

  



    
    

    
  
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;
>   }
>   /*

  

  



    
    

    
  


  
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.

  

  



    
    

    
  
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!

  

  



    
    

    
  


  
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+

    
    

    
  
Comment on attachment 95285 [details] [diff] [review]
Deallocate the string too

r=me

  

  



        Attachment #95285 -
        Flags: review+

    
    

    
  
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
        — 
      

    


  
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: