Closed Bug 649134 Opened 13 years ago Closed 13 years ago

Blank link rel causes subsequent conditional CSS to be displayed

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: cfrommann, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: html5, testcase)

Attachments

(3 files, 5 obsolete files)

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
Attached file Testcase (obsolete) —
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.
Attached file Testcase #2 (obsolete) —
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
Attached patch Like so? (obsolete) — Splinter Review
Attachment #525394 - Flags: review?(bzbarsky)
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
Attached file 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+
Attached patch part 1, fix <link> element (obsolete) — Splinter Review
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
> 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)
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-
Updated to fix merge conflicts in reftest.list
Attachment #526320 - Attachment is obsolete: true
(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+
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
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla6
Depends on: 667518
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.

Attachment

General

Created:
Updated:
Size: