Bug 42042

Summary: An empty value for xml:lang isn't considered
Product: WebKit Reporter: Rouven Weßling <me>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, eric, mitz, rwlbuis, webkit.review.bot
Priority: P2 Keywords: EasyFix, HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/REC-xml/#sec-lang-tag
Attachments:
Description Flags
Test case for xml:lang
none
Patch
none
Patch
none
correction patch mitz: review+

Description Rouven Weßling 2010-07-11 05:42:54 PDT
Created attachment 61175 [details]
Test case for xml:lang

According to the XML spec xml:lang="" is valid and declares that no language information is available. Webkit currently ignores empty values of xml:lang when applying styles. Firefox works as expected. I attached a test case.
Comment 1 mitz 2010-07-11 14:35:24 PDT
I think the issue here is that the CSSSelector::PseudoLang case of checkOneSelector() uses value.isEmpty() in a couple of places where it should use value.isNull().
Comment 2 Rob Buis 2010-07-15 13:33:07 PDT
Created attachment 61706 [details]
Patch

Matches FireFox and Opera behaviour for :lang and empty lang attributes.
Comment 3 mitz 2010-07-15 13:50:29 PDT
Comment on attachment 61706 [details]
Patch

> -                if (value.isEmpty() || !value.startsWith(argument, false))
> +                if (value.isNull() || !value.startsWith(argument, false))

Why is this change needed or desirable?
Comment 4 Rob Buis 2010-07-15 22:44:43 PDT
Hi Mitz,

(In reply to comment #3)
> (From update of attachment 61706 [details])
> > -                if (value.isEmpty() || !value.startsWith(argument, false))
> > +                if (value.isNull() || !value.startsWith(argument, false))
> 
> Why is this change needed or desirable?

Coming out of the loop, value.isEmpty() now means an acceptable value where we can test
the :lang selector against, and value.isNull() means no lang value found, so we can break.
Cheers,

Rob.
Comment 5 mitz 2010-07-15 23:49:21 PDT
(In reply to comment #4)
> Hi Mitz,
> 
> (In reply to comment #3)
> > (From update of attachment 61706 [details] [details])
> > > -                if (value.isEmpty() || !value.startsWith(argument, false))
> > > +                if (value.isNull() || !value.startsWith(argument, false))
> > 
> > Why is this change needed or desirable?
> 
> Coming out of the loop, value.isEmpty() now means an acceptable value where we can test
> the :lang selector against, and value.isNull() means no lang value found, so we can break.

Can you add a test for this?
Comment 6 Rob Buis 2010-07-16 09:28:28 PDT
Created attachment 61815 [details]
Patch

Adapted testcase to incorporate Mitz' request.
Comment 7 mitz 2010-07-16 10:18:29 PDT
I’m still confused. I updated to r63560, then I changed CSSStyleSelector.cpp:2616 back from isNull() to isEmpty() and built, then ran the test fast/css/lang-selector-empty-attribute.xhtml . The test still passes. What am I missing?
Comment 8 Rob Buis 2010-07-16 13:42:52 PDT
(In reply to comment #7)
> I’m still confused. I updated to r63560, then I changed CSSStyleSelector.cpp:2616 back from isNull() to isEmpty() and built, then ran the test fast/css/lang-selector-empty-attribute.xhtml . The test still passes. What am I missing?

I can confirm, sorry about that. I'll debug tomorrow, maybe fastGetAttribute returns something different then I assumed,...
Cheers,

Rob.
Comment 9 Adam Barth 2010-08-10 22:37:41 PDT
This appears to have been landed.
Comment 10 Alexey Proskuryakov 2010-08-10 22:46:18 PDT
Yes, <http://trac.webkit.org/changeset/63560>.

Rob, could you find out an answer to the question in comment 7?
Comment 11 Rob Buis 2010-08-11 12:28:57 PDT
Hi,

(In reply to comment #10)
> Yes, <http://trac.webkit.org/changeset/63560>.
> 
> Rob, could you find out an answer to the question in comment 7?

Right, sorry, did not get to it before my summer vacation.

Now that I look at it again, the change may not have been needed indeed, I mean the last
isEmpty -> isNull change in my patch.

Basically empty values (as well as null values) can never match the :lang selector, since at
least the way we parse it it can't be empty, i.e. length > 0, so the original way to break early there
seems fine to me. The only reason why that part of the patch worked, is that empty values will fail
on the next if statement and so a break will be done as before, just (unneededly so) a bit later.

I'll create a correcting patch soon.
Cheers,

Rob.
Comment 12 Rob Buis 2010-08-11 12:59:46 PDT
Created attachment 64152 [details]
correction patch
Comment 13 Rob Buis 2010-08-11 13:01:15 PDT
Reopening because comment 7 needs to cleared first.
Comment 14 mitz 2010-08-11 13:02:35 PDT
Comment on attachment 64152 [details]
correction patch

Thank you!
Comment 15 WebKit Review Bot 2010-08-11 14:40:49 PDT
http://trac.webkit.org/changeset/65182 might have broken Leopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/65182
http://trac.webkit.org/changeset/65183