Bug 77549 – Alternate xml-stylesheets with no title are loaded, in violation of the CSSOM draft

• Component: CSS
• Status: RESOLVED FIXED

[Boris Zbarsky]

See attached testcase.  The text should be green.

The relevant section of CSSOM is http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing-the-xml-stylesheet-processing-instruction steps 1 and 2:

  Let title be the value of the title pseudo-attribute or the empty string if the title
  pseudo-attribute is not specified.

  If there is an alternate pseudo-attribute whose value is a case-sensitive match for
  "yes" and title is the empty string terminate these steps.

[Alexey Proskuryakov]

There is no test case attached at the moment.

[Boris Zbarsky]

Created attachment 124965 [details]
Testcase

Odd.  I attached this when filing too....

[Alexey Proskuryakov]

See also: bug 24354.

[Dave Tharp]

Created attachment 129977 [details]
Patch

[Tom Zakrajsek]

Comment on attachment 129977 [details]
Patch

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

> Source/WebCore/dom/Document.h:474
> +    int pendingStylesheetCount() const { return m_pendingStylesheets; }

Hopefully, this is not necessary based on my comment in Source/WebCore/dom/ProcessingInstruction.cpp@138

> Source/WebCore/dom/ProcessingInstruction.cpp:138
> +        // the rules state that if this is an alternate stylesheet with no title, and another stylesheet has been

I don't see the part in the spec about "and another stylesheet has been previously loaded"

[Dave Tharp]

Created attachment 130198 [details]
Patch

[Antti Koivisto]

Comment on attachment 130198 [details]
Patch

Does this fix some real world issues? Does this match other browsers? Draft spec text is not a sufficient reason to change behavior.

[Dave Tharp]

(In reply to comment #7)
> (From update of attachment 130198 [details])
> Does this fix some real world issues? Does this match other browsers? Draft spec text is not a sufficient reason to change behavior.

It's a spec issue.  FF ignores xml alternate stylesheets without title attribute. It's a draft spec but follows published HTML spec for LINK attributes.

[Dave Tharp]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 130198 [details] [details])
> > Does this fix some real world issues? Does this match other browsers? Draft spec text is not a sufficient reason to change behavior.
> 
> It's a spec issue.  FF ignores xml alternate stylesheets without title attribute. It's a draft spec but follows published HTML spec for LINK attributes.

IE shows the same behavior as FF, in other words, IE respects the draft spec.

[Dave Tharp]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 130198 [details] [details] [details])
> > > Does this fix some real world issues? Does this match other browsers? Draft spec text is not a sufficient reason to change behavior.
> > 
> > It's a spec issue.  FF ignores xml alternate stylesheets without title attribute. It's a draft spec but follows published HTML spec for LINK attributes.
> 
> IE shows the same behavior as FF, in other words, IE respects the draft spec.

Regarding how this bug relates to bug 24354:  Both bugs have to do with behavior not explicitly defined in the W3C spec, but clarified in the CSSOM draft. FF and IE implement the behavior "if an alternate stylesheet has no title attribute, then ignore it". WebKit currently does not implement this behavior. This bug (77549) has to do with xml stylesheets, bug 24354 has to do with stylesheets specified in a link tag.

[Alexey Proskuryakov]

Comment on attachment 130198 [details]
Patch

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

> IE shows the same behavior as FF, in other words, IE respects the draft spec.

How did you test IE?

> Source/WebCore/dom/ProcessingInstruction.cpp:139
> +        // see http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing-the-xml-stylesheet-processing-instruction
> +        // The rules state that if this is an alternate stylesheet with no title, 
> +        // then this stylesheet should not be loaded.

No need for this comment. Implementing the spec while also following other browsers' behavior is the norm, not an exception that needs explanation.

> LayoutTests/ChangeLog:13
> +        * fast/css/xml-stylesheet-alternate-no-title-expected.txt: Added.
> +        * fast/css/xml-stylesheet-alternate-no-title.xhtml: Added.

This should be a text-only test. Please start with a template created by make-new-script-test, and use shouldBe with computed style to check that the text is green. Also, please check disabled state of the stylesheet object.

> LayoutTests/fast/css/xml-stylesheet-alternate-no-title.xhtml:8
> +      if (document.styleSheets.length > 1)
> +        document.styleSheets[1].disabled = false;

Why are you disabling the stylesheet here?

[Alexey Proskuryakov]

> Why are you disabling the stylesheet here?

OK, I see why.

[Alexey Proskuryakov]

I suggest also checking document.styleSheets.length with a shouldBe.

[Dave Tharp]

(In reply to comment #11)
> (From update of attachment 130198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130198&action=review
> 
> > IE shows the same behavior as FF, in other words, IE respects the draft spec.
> 
> How did you test IE?
> 
IE Was problematic in that clicking the the testcase link yielded clearly bad results (source rendered as text). To get around this, I copied the testcase locally, and gave it a xhtml extension. Loading the local file in IE showed that IE passed the testcase.

> > Source/WebCore/dom/ProcessingInstruction.cpp:139
> > +        // see http://dev.w3.org/csswg/cssom/#requirements-on-user-agents-implementing-the-xml-stylesheet-processing-instruction
> > +        // The rules state that if this is an alternate stylesheet with no title, 
> > +        // then this stylesheet should not be loaded.
> 
> No need for this comment. Implementing the spec while also following other browsers' behavior is the norm, not an exception that needs explanation.

OK, will remove the comment.
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/css/xml-stylesheet-alternate-no-title-expected.txt: Added.
> > +        * fast/css/xml-stylesheet-alternate-no-title.xhtml: Added.
> 
> This should be a text-only test. Please start with a template created by make-new-script-test, and use shouldBe with computed style to check that the text is green. Also, please check disabled state of the stylesheet object.

Ahh, a chance to explore a new tool :-) Will do. 
> 
> > LayoutTests/fast/css/xml-stylesheet-alternate-no-title.xhtml:8
> > +      if (document.styleSheets.length > 1)
> > +        document.styleSheets[1].disabled = false;
> 
> Why are you disabling the stylesheet here?

I see you added a comment saying you saw why. I'll disregard.

[Alexey Proskuryakov]

> IE Was problematic in that clicking the the testcase link yielded clearly bad results (source rendered as text). To get around this, I copied the testcase locally, and gave it a xhtml extension. Loading the local file in IE showed that IE passed the testcase.

When I do this, I see black text. And when I change the test to use real CSS files instead of data URLs, the text is red.

I'm testing with IE9.

[Dave Tharp]

(In reply to comment #15)
> > IE Was problematic in that clicking the the testcase link yielded clearly bad results (source rendered as text). To get around this, I copied the testcase locally, and gave it a xhtml extension. Loading the local file in IE showed that IE passed the testcase.
> 
> When I do this, I see black text. And when I change the test to use real CSS files instead of data URLs, the text is red.
> 
> I'm testing with IE9.
I'll spend some more time testing on IE. It's weird that IE mishandles the testcase link, and now hearing your experience testing it, its clear the IE test needs some more diligence.  I'll be re-writing the test (as per your direction) in the morning, and I will be sure to heavily test IE (to show that it handles the alternate stylesheets the same as FF) before I submit the patch.

[Dave Tharp]

Created attachment 130904 [details]
Patch

[Dave Tharp]

(In reply to comment #17)
> Created an attachment (id=130904) [details]
> Patch

- removed comment in code
- re-wrote test as a text test

Regarding the IE testing. My original testing was on IE 8.  When the testcase was saved to a local .xhtml file, I observed the test case pass.  In order to investigate your testing results on IE 9, I upgraded to IE 9, and verified your black text result. I have also verified that IE appears to be loading both stylesheets (it should ignore the second), but not applying any stylesheet. In short, IE 9 is not compliant.  FF, on the other hand, passes the test case properly, and it passes the newly written text layout test.

[Alexey Proskuryakov]

If Firefox is the only "compliant" browser, then it sounds like a mistake in draft spec.

[Jonathan Watt]

Hi guys. This issue was originally brought to Mozilla's attention because http://ieblog.members.winisp.net/images/20111027-blog_map.html didn't work in Firefox due to it having alternate style sheets without titles. I approached the IE team on Mozilla's behalf, and they agreed that the example was wrong to do that, and that IE should have been treating the example the same as Firefox. They fixed the test (so it now works in Firefox) and I expect a fix for IE (similar to the one up for review here) in IE 10.

(In reply to comment #19)
> If Firefox is the only "compliant" browser, then it sounds like a mistake in draft spec.

My understanding is that it is very deliberate, on the basis that alternative style sheets are intended to be presented to the user in the UI for them to select between, and without a title it's not possible to provide the user with an informative description of the alternative style. If that's not sufficient reasoning, I'm sure others from Mozilla can give you a lot more info than I can.

[Dave Tharp]

(In reply to comment #20)
> Hi guys. This issue was originally brought to Mozilla's attention because http://ieblog.members.winisp.net/images/20111027-blog_map.html didn't work in Firefox due to it having alternate style sheets without titles. I approached the IE team on Mozilla's behalf, and they agreed that the example was wrong to do that, and that IE should have been treating the example the same as Firefox. They fixed the test (so it now works in Firefox) and I expect a fix for IE (similar to the one up for review here) in IE 10.
> 
> (In reply to comment #19)
> > If Firefox is the only "compliant" browser, then it sounds like a mistake in draft spec.
> 
> My understanding is that it is very deliberate, on the basis that alternative style sheets are intended to be presented to the user in the UI for them to select between, and without a title it's not possible to provide the user with an informative description of the alternative style. If that's not sufficient reasoning, I'm sure others from Mozilla can give you a lot more info than I can.

I'm not happy that IE 9 behaves differently than IE 8 with regards to this issue. However, the test suite referred to by bug 24354 is testing the same behavior (though not in xml style sheets).  Note that IE 9 passes all 5 of these tests (thus complying with the spec).  Therefore I'd argue that IE 9 is mishandling the application of xml stylesheets. Indeed, if you google "IE 9 not loading xhtml", you'll see many queries along these lines.

[Boris Zbarsky]

Opera passes the testcase, as far as I can tell.

And iirc IE passes it with HTML <link> elements; it's just confused in the XML case.  So for <link> all non-webkit browsers agree here.  Seems to me like the xml-stylesheet and html:link cases should have identical behavior (and this is what the spec says right now).

[Dave Tharp]

(In reply to comment #22)
> Opera passes the testcase, as far as I can tell.
> 
> And iirc IE passes it with HTML <link> elements; it's just confused in the XML case.  So for <link> all non-webkit browsers agree here.  Seems to me like the xml-stylesheet and html:link cases should have identical behavior (and this is what the spec says right now).
Agreed.  bug 24354 has to do with <link>, and I've verified that IE passes the testcases attached to that bug.

[Dave Hyatt]

Comment on attachment 130904 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 130904 [details]
Patch

Rejecting attachment 130904 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
webkit-commit-queue/Source/WebKit/chromium/webkit --revision 125600 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 125600.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11942734

[Tom Zakrajsek]

Manually Committed r110632: <http://trac.webkit.org/changeset/110632>

Closing bug