74281 - inline style sheet's href is document url (should be null)

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
 1.  Load http://www.squarefree.com/bookmarklets/
 2a. Type into the location bar: javascript:alert(document.styleSheets[0].href)
 2b. Type into the location bar: javascript:document.styleSheets[0].href

Result:
 2a: See document's href in an alert
 2b: See document's href as a new document

Expected: 
 2a: See "null" in the alert.
 2b: Nothing should happen, since the function should have returned null.

Note: 
 IE has document.styleSheets[0].href return an empty string.  I think that's
 incorrect, since the spec says it should be null.

Spec:
 http://www.w3.org/TR/DOM-Level-2-Style/stylesheets.html#StyleSheets-StyleSheet

Comment 1

[Johnny Stenback (:jst)]

Not a showstopper currently, aiming for mozilla1.0.

Comment 2

[Fabian Guisset]

jst, as long as strings can't be null this can't be implemented, correct?
I have a patch that implements IE's behavior (i.e. returns an empty string when
the stylesheet is inline) in my tree.

Comment 3

[Johnny Stenback (:jst)]

Fabian, you're right, but make your patch call SetDOMStringToNull(theString);
and we could check this in, for now it'll only make the string empty, but once
we actually support null strings things will work as specified.

Comment 4

[Fabian Guisset]

Attachment: Proposed fix for bug 74281. Set href to null for inline stylesheets.

Comment 5

[Fabian Guisset]

peterv, glazou, comments on the patch please?

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 47848 [details] [diff] [review]
Proposed fix for bug 74281. Set href to null for inline stylesheets.

>+    if(is_inline) {

Nit: add a space after the if. r=peterv.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 47848 [details] [diff] [review]
Proposed fix for bug 74281. Set href to null for inline stylesheets.

What peterv said, plus, change mDocument->GetBaseURL() to mDocument->GetDocumentURL().

sr=jst

Comment 8

[Peter Van der Beken]

jst, we've been through this before ;). See
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSLoader.cpp#1296.

Comment 9

[Johnny Stenback (:jst)]

Hmm, ok, that seems wrong, but I'm ok with this patch as is then (appart from
the missing space).

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

don't forget a whitespace after if in  if(!mDocument)
r=glazman

Comment 11

[Hixie (not reading bugmail)]

Technically, this patch is wrong.

If foo.xml contains:

   <?xml-stylesheet href="foo.xml"?>
   <yasl xmlns="http://style.example.org/yasl/">
     <select target=":root">
       <color="green"/>
     </select>
   </yasl>
...where yasl is yet another style language, this one in XML, then the stylesheet
of that document should have an href even though it is the same URI as the 
document itself.

A contrived case, I agree, but if we're going to be compliant...

Comment 12

[Fabian Guisset]

I'll let you argue with glazou peterv & jst, I'm not qualified to comment on
that ;-)

Comment 13

[Fabian Guisset]

It occured to me that my patch changes nsCSSStyleSheet:GetHref(), and your
example, Hixie, uses another style language. So ns_CSS_StyleSheet shouldn't be
called in the first place.

Comment 14

[Hixie (not reading bugmail)]

But as I pointed out on IRC, it wouldn't work for a case where the URI of the
document and the stylesheet were the same, even though they were different files.
e.g., http://www.hixie.ch/tests/evil/dom/css/001.cgi

Note that we currently pass this test as far as this bug is concerned (although we
do fail to get the right CSS file, unlike IE which passes all versions of this 
test -- including the cached one).

Comment 15

[Peter Van der Beken [:peterv]]

To handle that correctly we probably need to rewrite part of the CSS loader.

Comment 16

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 17

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Sorta working patch if anyone cares

It's got some irrelevant stuff in it and won't compile in debug mode ('cause of
const issues), but it's the general idea.

Comment 19

[Boris Zbarsky [:bzbarsky]]

The patch for bug 397427 fixed this.  And the test checked in for that bug covers this bug as well.