Bug 77549 - Alternate xml-stylesheets with no title are loaded, in violation of the CSSOM draft
Summary: Alternate xml-stylesheets with no title are loaded, in violation of the CSSOM...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Tharp
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-01 06:30 PST by Boris Zbarsky
Modified: 2012-03-13 16:11 PDT (History)
5 users (show)

See Also:


Attachments
Testcase (406 bytes, text/xml)
2012-02-01 10:22 PST, Boris Zbarsky
no flags Details
Patch (4.61 KB, patch)
2012-03-02 16:23 PST, Dave Tharp
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2012-03-05 13:50 PST, Dave Tharp
no flags Details | Formatted Diff | Diff
Patch (5.33 KB, patch)
2012-03-08 14:22 PST, Dave Tharp
hyatt: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2012-02-01 06:30:07 PST
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.
Comment 1 Alexey Proskuryakov 2012-02-01 09:53:58 PST
There is no test case attached at the moment.
Comment 2 Boris Zbarsky 2012-02-01 10:22:05 PST
Created attachment 124965 [details]
Testcase

Odd.  I attached this when filing too....
Comment 3 Alexey Proskuryakov 2012-02-01 11:21:40 PST
See also: bug 24354.
Comment 4 Dave Tharp 2012-03-02 16:23:12 PST
Created attachment 129977 [details]
Patch
Comment 5 Tom Zakrajsek 2012-03-05 10:22:48 PST
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"
Comment 6 Dave Tharp 2012-03-05 13:50:55 PST
Created attachment 130198 [details]
Patch
Comment 7 Antti Koivisto 2012-03-05 14:38:08 PST
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.
Comment 8 Dave Tharp 2012-03-05 14:46:38 PST
(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.
Comment 9 Dave Tharp 2012-03-05 16:36:11 PST
(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.
Comment 10 Dave Tharp 2012-03-07 09:25:47 PST
(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.
Comment 11 Alexey Proskuryakov 2012-03-07 15:44:31 PST
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?
Comment 12 Alexey Proskuryakov 2012-03-07 15:49:39 PST
> Why are you disabling the stylesheet here?

OK, I see why.
Comment 13 Alexey Proskuryakov 2012-03-07 15:52:02 PST
I suggest also checking document.styleSheets.length with a shouldBe.
Comment 14 Dave Tharp 2012-03-07 16:10:21 PST
(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.
Comment 15 Alexey Proskuryakov 2012-03-07 16:29:39 PST
> 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.
Comment 16 Dave Tharp 2012-03-07 19:51:56 PST
(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.
Comment 17 Dave Tharp 2012-03-08 14:22:16 PST
Created attachment 130904 [details]
Patch
Comment 18 Dave Tharp 2012-03-08 14:28:32 PST
(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.
Comment 19 Alexey Proskuryakov 2012-03-08 14:45:28 PST
If Firefox is the only "compliant" browser, then it sounds like a mistake in draft spec.
Comment 20 Jonathan Watt 2012-03-08 15:00:14 PST
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.
Comment 21 Dave Tharp 2012-03-08 15:06:46 PST
(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.
Comment 22 Boris Zbarsky 2012-03-08 20:58:35 PST
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).
Comment 23 Dave Tharp 2012-03-09 08:57:02 PST
(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.
Comment 24 Dave Hyatt 2012-03-13 11:16:43 PDT
Comment on attachment 130904 [details]
Patch

r=me
Comment 25 WebKit Review Bot 2012-03-13 15:32:58 PDT
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
Comment 26 Tom Zakrajsek 2012-03-13 16:11:33 PDT
Manually Committed r110632: <http://trac.webkit.org/changeset/110632>

Closing bug