Bug 16801 – CSS2: :lang has possibly poor performance on large pages

• Status: ASSIGNED
• Component: CSS

[Eric Seidel (no email)]

CSS2: :lang has possibly poor performance on large pages

Proper :lang support was added as part of bug 9454.  Hyatt raised concerns about possible performance issues with :lang on large pages.  This bug is to track any such issues.

[Nicholas Shanks]

MacDome, to address an IRC comment, maybe we could do a survey of bilingual Chinese/Japanese readers and find out how often wrong font selection affects them. I would presume that han unification on bilingual pages is one of the biggest problems that :lang() solves. We should probably have some information oh how often the selector is used on such pages and how deep their dom trees generally are.

[Eric Seidel (no email)]

Actually, I realize that our implementation actually needs to change to allow fonts to use this information.  Currently the cascaded lang value is never stored anywhere, which means that other features (like font chosing, or SVG stuff) can't use this value, they'd have to recompute it themselves.

[Dave Hyatt]

Yeah, that's why we should have the information in the style so that it is readily available. :)

<rdar://problem/6175111>

[Jungshik Shin]

Not having noticed this bug blocking bug 10874, I uploaded a patch (along the direction discussed here) to bug 21312. I'll move it here. 

[Jungshik Shin]

Created attachment 28417 [details]
tentative patch 

I don't think this patch is acceptable as it is (it even has a commented-out block), but I'd like to see whether I'm in the right direction. This patch is copied from bug 21312.

[Jungshik Shin]

Comment on attachment 28417 [details]
tentative patch 

Darin made an extensive comment on this patch (bug 21312 comment 5).

[Jungshik Shin]

I found that '-webkit-locale' (CSS property) was introduced. I don't understand why we introduced '-webkit-locale' when we have 'lang' to determine what locale to use for  hyphenation, line breaking, text-transform,  font selection, etc.  

However, in retrospect, introducing '-webkit-locale' seems to be a brilliant move because  Darin's comment on my patch for bug 21312 are mostly addressed by mapping HTML lang attribute to CSS '-webkit-locale' as HTML 'dir' is to CSS 'direction'.

[Jungshik Shin]

Created attachment 100726 [details]
a one-line patch (that does not work)

This patch is a 1-liner that tried to map 'lang' attribute to '-webkit-locale'. I also have a layout test but it's not included here because it's not working yet. 

If it works, it'd be great, but it does not. 

addCSSProperty calls CSSParser::parseValue() eventually and it fails because |cssparse| (in CSSGrammar.cpp) sets m_numParsedProperties to 0.

The following part in CSSGrammar.y is reached. The 3rd entry (STRING maybe_space) should be hit, but instead 'IDENT maybe_space' is used. Because lang/locale id (like "he", "en-US", "zh", "fr")  is not a cssValueKeyword, it fails. 

term:
  unary_term { $$ = $1; }
  | unary_operator unary_term { $$ = $2; $$.fValue *= $1; }
  | STRING maybe_space { $$.id = 0; $$.string = $1; $$.unit = CSSPrimitiveValue::CSS_STRING; }
  | IDENT maybe_space {
      $$.id = cssValueKeywordID($1);      $$.unit = CSSPrimitiveValue::CSS_IDENT;
      $$.string = $1;  }


I also considered special-casing -webkit-locale like font-related properties are in C++, but if I can tweak CSSGrammar.y to do what's necessary, I'd rather take that route. 


http://paste.lisp.org/display/123254

(In reply to comment #9)

> addCSSProperty calls CSSParser::parseValue() eventually and it fails because |cssparse| (in CSSGrammar.cpp) sets m_numParsedProperties to 0.
> 
> The following part in CSSGrammar.y is reached. The 3rd entry (STRING maybe_space) should be hit, but instead 'IDENT maybe_space' is used. Because lang/locale id (like "he", "en-US", "zh", "fr")  is not a cssValueKeyword, it fails. 

Locale identifiers are not CSS keywords. When you specify a -webkit-locale value you need to either specify the auto keyword or a string whose content is the locale identifier. This means that you need to add quotation marks around the attribute value (and escape internal quotation marks, maybe).

I.e. these are valid:

-webkit-locale: auto;
-webkit-locale: "en_US";

This is not:
-webkit-locale: en_US;

[Jungshik Shin]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > addCSSProperty calls CSSParser::parseValue() eventually and it fails because |cssparse| (in CSSGrammar.cpp) sets m_numParsedProperties to 0.
> > 
> > The following part in CSSGrammar.y is reached. The 3rd entry (STRING maybe_space) should be hit, but instead 'IDENT maybe_space' is used. Because lang/locale id (like "he", "en-US", "zh", "fr")  is not a cssValueKeyword, it fails. 
> 
> Locale identifiers are not CSS keywords. 

That much I knew. My question was why it's not picked up as a string. 

> When you specify a -webkit-locale value you need to either specify the auto keyword or a string whose content is the locale identifier. This means that you need to add quotation marks around the attribute value (and escape internal quotation marks, maybe).

Thank you. Now I got it. I should have quote *explicitly* the value of 'lang'.  Otherwise, the value will be treated as a CSS keyword and parsing will fail because it's not.

[Jungshik Shin]

Created attachment 100910 [details]
wip patch (partly working)

xml:lang is not taken into account. Content-Language is not, either. 
More importantly, it crashes a bunch of layout tests (when computeInheritedLanguage() is called). I haven't yet looked into this. 

fast/selectors/lang-inheritance2.html  
css1/conformance/forward_compatible_parsing.html       
http/tests/eventsource/workers/eventsource-simple.html 
svg/W3C-SVG-1.1/styling-css-05-b.svg   
fast/dom/css-selectorText.html 
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml     
fast/selectors/lang-vs-xml-lang-xhtml.xhtml    
fast/selectors/lang-inheritance.html   
http/tests/misc/acid3.html     
fast/selectors/lang-vs-xml-lang.html   
fast/css/css3-modsel-22.html   
fast/css/lang-selector-empty-attribute.xhtml   
fast/css/css-selector-text.html        
fast/css/css3-space-in-nth-and-lang.html       
css2.1/t0402-c71-fwd-parsing-02-f.html 
inspector/debugger/script-formatter.html


Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000038
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000101472798 WTF::RefPtr<WebCore::StyleRareInheritedData>::get() const + 12 (RefPtr.h:60)
1   com.apple.WebCore             	0x0000000101501305 WebCore::DataRef<WebCore::StyleRareInheritedData>::get() const + 21 (DataRef.h:33)
2   com.apple.WebCore             	0x000000010150131d WebCore::DataRef<WebCore::StyleRareInheritedData>::operator->() const + 21 (DataRef.h:36)
3   com.apple.WebCore             	0x00000001014727dd WebCore::RenderStyle::locale() const + 25 (RenderStyle.h:703)
4   com.apple.WebCore             	0x00000001016e2ef9 WebCore::Element::computeInheritedLanguage() const + 109 (Element.cpp:1778)
5   com.apple.WebCore             	0x0000000101526f08 WebCore::CSSStyleSelector::SelectorChecker::checkOneSelector(WebCore::CSSSelector*,

[Jungshik Shin]

Created attachment 101041 [details]
wip patch (only partly working)

No more crash with null check, but still not taking care of  xml:lang vs lang precedence. So it breaks tests like fast/selectors/lang-vs-xml-lang-xhtml.xhtml

[Jungshik Shin]

Created attachment 101187 [details]
wip patch (almost works)

This one works except when xml:lang is specified without lang specified.

[Jungshik Shin]

Comment on attachment 101187 [details]
wip patch (almost works)

View in context: https://bugs.webkit.org/attachment.cgi?id=101187&action=review

> Source/WebCore/dom/Element.cpp:1777
> +            RefPtr<RenderStyle> nodeStyle(static_cast<const Element*>(n)->renderStyle());

Instead of the above, I wonder if the following is better:

RefPtr<RenderStyle> nodeStyle(const_cast<Element*>(static_cast<const Element*>(n))->computedStyle(NOPSEUDO));

Note that computedStyle() is not const. So, either I have to const_cast or drop 'const' for this function. If there's no difference, I'd just use what I have in the patch.

> Source/WebCore/html/HTMLElement.cpp:164
> +        // FIXME: this does not work when xml:lang is present without lang.

XHTML spec says that both lang and xml:lang have to be used (for backward compatibility). I guess there will be very few document with only xml:lang. And, none of layout tests test that case. So, maybe it's ok not to support cases when ONLY xml:lang is specified. 

I tried the following, but it's never hit. 

else if (attr->name() == XMLNames::langAttr)

[Jungshik Shin]

Created attachment 105545 [details]
a test (not for review)

I'm trying to write a layout test that just checks the value of 'style.webkitLocale' of a node with 'lang' set directly or by inheritance. However, somehow accessing <node>.style.webkitLocale in JavaScript does not work as expected. 

In DOM inspector, all the nodes with lang set directly or by inheritance do have '-webkit-locale' set to the value I used. However, 'lang' node in the attached HTML file has

l1= l2= l3= l4=undefined.

In case of l4 (for 'l4' node), '-webkit-locale' is specified in inline style spec. In JS console, document.getElemetnById("l4".style.webkitLocale is "ja" as expected. 

I've tried HTML 'dir' mapped to CSS 'direction'.  Nodes with HTML dir attribute have 'direction' set to the value of 'dir' in DOM inspector. However, in JavaScript, their values are empty (the same as HTML 'lang' mapped to CSS '-webkit-locale').  However, a node with 'style="dir: rtl"' works as expected. That is, <node>.style.direction has 'rtl' instead of 'undefined'.  


Perhaps, some more plumbing is necessary to expose mapped HTML attributes in JavaScript?  Any hint/help would be appreciated. In the meantime, I'll poke around further.

[Jungshik Shin]

Oh. I think I should use computedStyle, instead.

[Jungshik Shin]

Created attachment 106075 [details]
patch with layout test

[WebKit Review Bot]

Comment on attachment 106075 [details]
patch with layout test

Attachment 106075 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9580465

New failing tests:
fast/css/css3-modsel-22.html
fast/css/lang-selector-empty-attribute.xhtml
svg/W3C-SVG-1.1/styling-css-05-b.svg
fast/selectors/lang-vs-xml-lang.html
fast/css/css3-space-in-nth-and-lang.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
fast/selectors/lang-inheritance.html

[Jungshik Shin]

Comment on attachment 106075 [details]
patch with layout test

it can't be landed because a bunch of layout tests involving psuedo-lang selector fails. 

When they're loaded for the 1st time, they're broken. As soon as I turn on DOM inspector, all the nodes affected by pseudo-lang selector get rendered correctly.

[Jungshik Shin]

Created attachment 106227 [details]
patch update (still not passing some layout tests)

I also tried using computedStyle in computeInheritedLanguage, but it still does not pass some layout tests. 

What I'll do is to punt this bug (making computeInheritedLanguage more efficient) and file a new bug that focus on mapping 'lang' and 'xml:lang' to -webkit-locale because only the latter is necessary for font fallback.  
That means Content-Langauge is not reflected in font fallback (neither will be document encoding). I have another idea on that issue, but we can punt that, too, for now. 

After filing a new bug on mapping lang/xml:lang to -webkit-locale, I'll upload a patch for html/HTMLElement.cpp there along with the current layout test.

[Jungshik Shin]

I filed bug 67586 with a narrower scope than this. To that bug, I attached a patch to map lang/xml:lang to -webkit-locale to be used in font fallback and tex-transform.