Bug 88157 – <link> with non-CSS type should not be retrieved

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Boris Zbarsky]

Created attachment 145414 [details]
Testcase

The attached testcase should render green text.  In WebKit it renders red.

Relevant spec is at http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#the-link-element and says:

  For external resource links, the type attribute is used as a hint to user agents so that they can
  avoid fetching resources they do not support.
  ...
  If the UA does not support the given MIME type for the given link relationship, then the UA
  should not obtain the resource;

WebKit's behavior here is not a spec violation per se, since this is a SHOULD level requirement, not a MUST.  But unless there's a really good reason to violate the SHOULD it's probably better to follow it.  Other UAs do.

[Sumedha Widyadharma]

Created attachment 146026 [details]
Patch to check if a link-elements type is text/css or not set before fetching

[Rob Buis]

Created attachment 385982 [details]
Patch

[Rob Buis]

Created attachment 385986 [details]
Patch

[Rob Buis]

Created attachment 385990 [details]
Patch

[youenn fablet]

Comment on attachment 385990 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:278
> +    bool treatAsStyleSheet = (m_relAttribute.isStyleSheet && (m_type.isNull() || m_type.containsIgnoringASCIICase("text/css")))

What happens for "ABCtext/css", "text/cssDEF"?
Should we have more edge case tests.
Might be nice to have these as WPT tests as well and verify we match other browser behaviours.

[Rob Buis]

Created attachment 386468 [details]
Patch

[Rob Buis]

Created attachment 386471 [details]
Patch

[Rob Buis]

Created attachment 386601 [details]
Patch

[Rob Buis]

Created attachment 386602 [details]
Patch

[Rob Buis]

(In reply to youenn fablet from comment #5)
> Comment on attachment 385990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385990&action=review
> 
> > Source/WebCore/html/HTMLLinkElement.cpp:278
> > +    bool treatAsStyleSheet = (m_relAttribute.isStyleSheet && (m_type.isNull() || m_type.containsIgnoringASCIICase("text/css")))
> 
> What happens for "ABCtext/css", "text/cssDEF"?
> Should we have more edge case tests.
> Might be nice to have these as WPT tests as well and verify we match other
> browser behaviours.

The tests are now added to the WPT repo:
https://github.com/web-platform-tests/wpt/pull/20930

I imported them into this patch.

[WebKit Commit Bot]

Comment on attachment 386602 [details]
Patch

Clearing flags on attachment: 386602

Committed r253992: <https://trac.webkit.org/changeset/253992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58293948>

[Darin Adler]

Comment on attachment 386602 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:287
> +        treatAsStyleSheet = document().settings().treatsAnyTextCSSLinkAsStylesheet() && m_type.containsIgnoringASCIICase("text/css");

Calling "contains" here can’t possibly be exactly right, since it’s just a substring check, not checking for delimiters or anything. Not new, but not correct.

[Rob Buis]

(In reply to Darin Adler from comment #14)
> Comment on attachment 386602 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386602&action=review
> 
> > Source/WebCore/html/HTMLLinkElement.cpp:287
> > +        treatAsStyleSheet = document().settings().treatsAnyTextCSSLinkAsStylesheet() && m_type.containsIgnoringASCIICase("text/css");
> 
> Calling "contains" here can’t possibly be exactly right, since it’s just a
> substring check, not checking for delimiters or anything. Not new, but not
> correct.

But does the check not make sense together with treatsAnyTextCSSLinkAsStylesheet setting? What is the history behind the setting and is it not needed anymore? Anyway, will investigate.