An empty value for xml:lang isn't considered

[Rouven Weßling]

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.

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

[Rob Buis]

Created attachment 61706 [details]
Patch

Matches FireFox and Opera behaviour for :lang and empty lang attributes.

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?

[Rob Buis]

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.

(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?

[Rob Buis]

Created attachment 61815 [details]
Patch

Adapted testcase to incorporate Mitz' request.

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?

[Rob Buis]

(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.

[Adam Barth]

This appears to have been landed.

[Alexey Proskuryakov]

Yes, <http://trac.webkit.org/changeset/63560>.

Rob, could you find out an answer to the question in comment 7?

[Rob Buis]

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.

[Rob Buis]

Created attachment 64152 [details]
correction patch

[Rob Buis]

Reopening because comment 7 needs to cleared first.

Comment on attachment 64152 [details]
correction patch

Thank you!

[WebKit Review Bot]

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