Blank link rel causes subsequent conditional CSS to be displayed

RESOLVED FIXED in mozilla6

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cfrommann, Assigned: mats)

Tracking

({html5, testcase})

unspecified
mozilla6
html5, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; InfoPath.2; .NET CLR 1.1.4322; .NET CLR 3.0.30729; .NET CLR 3.5.30729; .NET4.0C; MS-RTC LM 8)
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0) Gecko/20100101 Firefox/4.0

If there is a blank stylesheet link reference anywhere in a document, all subsequent CSS except the first selector (regardless of whether or not it's in a conditional comment) is rendered.

e.g.

<link rel="stylesheet" type="text/css" href="" />
<!--[if lte IE 8]>
	<style type="text/css">
		#foo {
			/* This doesn't get evaluated */
		}
		#ie {
			border: 5px solid red;
		}
		#ie {
			display: block;
		}
		#moz {
			display: none;
		}
	</style>
<![endif]-->

#foo is not rendered, subsequent rules are

Reproducible: Always

Steps to Reproduce:
1. Include a link rel with a blank href
2. Include subsequent css in a conditional comment
Actual Results:  
All but the first rule will be displayed, despite the conditional comment

Expected Results:  
The rules will only be rendered if the conditions are met
(Assignee)

Comment 1

6 years ago
Created attachment 525276 [details]
Testcase
(Assignee)

Comment 2

6 years ago
This is fun stuff :-)

The href="" is a reference to the document itself, so it loads that
as text/css and finds that everything leading up to the first {
is a bogus CSS selector and thus discards the first rule, then there's
three valid rules and then some junk at the end...

As far as I know this works as expected, unless we want to make an
exception here like we did in some other case... was it for images?
Keywords: testcase
The spec changed the conformance requirements to require a non-empty URL there: http://www.whatwg.org/specs/web-apps/current-work/#attr-link-href

It's not clear to me if the spec was intended also to ignore empty URLs in processing. Currently, AFAICT, the spec doesn't have the right wording to say that if it was intended.
(Assignee)

Comment 4

6 years ago
Created attachment 525391 [details]
Testcase #2
(Assignee)

Comment 5

6 years ago
I think it's quite clear [that it should be ignored] in the steps to obtain the resource a few paragraphs down.  The first step is "If the href attribute's value is the empty string, then abort these steps.".  I think this implicitly
also applies to attribute values with only whitespace;  it follows from
"valid non-empty URL potentially surrounded by spaces" at the link you gave.

Google Chrome 12 loads it.  Opera 11, Firefox 3.6.x and IE9 ignores it.
Status: UNCONFIRMED → NEW
Component: General → DOM: Core & HTML
Ever confirmed: true
Keywords: html5, regression
OS: Windows Vista → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
(Assignee)

Comment 6

6 years ago
Created attachment 525394 [details] [diff] [review]
Like so?
Attachment #525394 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

6 years ago
Actually, Firefox 3.6.x and IE9 also loads it - using the testcase
attachment links gave the wrong result since it has a query string in it.
So, it's only Opera 11 that ignores it.
Keywords: regression
(Assignee)

Comment 8

6 years ago
Created attachment 525405 [details]
Testcase

Bah, I have to correct myself again.  IE9 does in fact ignore the empty href.
Here's a better testcase that removes the <style> and IE-conditional markup.
(still have to save it to a local file)

So, Opera and IE9 ignores it, Chrome and Firefox 3.6.x loads it.
Attachment #525276 - Attachment is obsolete: true
Attachment #525391 - Attachment is obsolete: true
Comment on attachment 525394 [details] [diff] [review]
Like so?

Do we need something similar for Link HTTP headers?

There's no point in doing the HasAttr check.  Just do the GetAttr call and then do the whitespace thing... and you probably need to file a bug on the spec about whitespace.

r=me with those nits.
Attachment #525394 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 526320 [details] [diff] [review]
part 1, fix <link> element

Nit fixed.  Simplified the reftest somewhat.
Filed HTML spec bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=12508
Assignee: nobody → matspal
Attachment #525394 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 526329 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

> Do we need something similar for Link HTTP headers?

Maybe.  The HTTP 1.1 definition of space [1] is different from HTML5's -
it doesn't include \f.  So a space-only URI reference in a Link field
that includes \f would be loaded (see testcases in this patch).

[1] http://tools.ietf.org/html/rfc2616#section-2.2
Attachment #526329 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
Comment on attachment 526329 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

This seems to be missing test_bug649134.html.  And why do you need the separate directory?  Just putting the new files in content/html/content/test should work, right?
Attachment #526329 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 13

6 years ago
Created attachment 527614 [details] [diff] [review]
part 1, fix <link> element

Updated to fix merge conflicts in reftest.list
Attachment #526320 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 527615 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

(In reply to comment #12)
> This seems to be missing test_bug649134.html.

Sorry about that.

> And why do you need the separate directory? Just putting the new files
> in content/html/content/test should work, right?

It's needed for the http://mochi.test server to fail without the patch.
Here's how the test works - when the bug649134/file_bug649134-*.sjs
file is loaded, it responds with the white-space Link: header
which we will now ignore and not load, this part would work without
the extra directory.

Without the fix, we would load the directory path where the .sjs is
located http://mochi.test:8888/tests/content/html/content/test/
but the mochitest server handles such URLs internally and responds
with the list of tests to run, but there's a way to avoid that:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#2464
Attachment #526329 - Attachment is obsolete: true
Attachment #527615 - Flags: review?(bzbarsky)
Comment on attachment 527615 [details] [diff] [review]
part 2, fix HTTP Link field for HTML documents

OK.  Please just do this for all documents (I don't see why the Link header handling should depend on the type of document), and add a comment in the file explaining why the extra subdirectory is needed, and r=me.
Attachment #527615 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/mozilla-central/rev/f3c01fd55b6b
http://hg.mozilla.org/mozilla-central/rev/a5b0d8686c97
http://hg.mozilla.org/mozilla-central/rev/cdf3154afc0f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
(Assignee)

Updated

6 years ago
Depends on: 667518
(Assignee)

Comment 17

6 years ago
Filed bug 667518, based on the "Editor's Response" in 
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12508#c2
which says we should load a non-empty white-space-only @href.
You need to log in before you can comment on or make changes to this bug.