Relative URIs in PIs in overlays are resolved relative to the root document and not relative to the overlay the PI is in.

RESOLVED FIXED in mozilla1.9alpha2

Status

()

Core
XUL
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Philip Chee, Assigned: Nickolay_Ponomarev)

Tracking

({regression, testcase})

Trunk
mozilla1.9alpha2
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Related Bug 360863

Looks like there is somewhere in the overlay engine that should be using:
mCurrentPrototype->overlayURI
instead of
mDocumentURI

Quote:
noticed that the prefbarslimbuttons option in prefbar didn't work with the 3.0a1 release after bumping the maxVersion of prefbar. After some checking with the DOM Inspector I noticed that the prefbarOverlay.css style sheet had a wrong url:
<?xml-stylesheet href="prefbarOverlay.css" type="text/css"?>

That seemed to work in older versions of Fx, but this release needs the full path:
<?xml-stylesheet href="chrome://prefbar/content/prefbarOverlay.css"
type="text/css"?>

Quote:
It looks like a bug to me.
If I enter a wrong path then I see an error in the Error Console.
With a relative path then there is no error. So maybe the part that parses the xul file can find the file, but doesn't save the name with the full path (only the relative name) and later the part that tries to load the content expands the path, but obviously doesn't know where the file is located and thus fails. The DOMi shows a wrong path (chrome://browser/content/) with no (0) rules loaded.
(Assignee)

Comment 2

12 years ago
Well, since the PIs are now inserted in the document, they are loaded as if they were in the base document to begin with. This actually makes more sense than merging them in the base document, but using the overlay's URI as the base URI.

I guess we want to revert to the old behavior though to avoid breaking extensions. Right?

Note that <script /> also resolves its URI against the overlay's URI, so it appears that the old behavior was coded that way on purpose. If we decide to keep the new behavior, <script /> case needs to be changed to be consistent with PIs case. It will break pretty much every extension though...
Assignee: jag → asqueella
Keywords: regression
(Assignee)

Updated

12 years ago
Blocks: 319654
(Assignee)

Comment 3

12 years ago
Oh, and I can't reproduce this:

> If I enter a wrong path then I see an error in the Error Console.
> With a relative path then there is no error. So maybe the part that parses the
> xul file can find the file, but doesn't save the name with the full path

I always get an error with an overlay PI and never with a stylesheet PI. More details?
Could add a method on nsIStyleLinkElement or whatever to set a base URI...  and use that here.
Flags: blocking1.9?
(Assignee)

Comment 5

12 years ago
Created attachment 248224 [details]
mozilla/layout/reftests/xul-document-load/subdir

Not being able to generate patches for new subdirectories sucks.
(Assignee)

Comment 6

12 years ago
Created attachment 248226 [details] [diff] [review]
patch [not for checkin]

This works but feels like a hack :( Wherever we don't do special processing, relative URIs will mean relative to the document URI.

This won't build without the patch in bug 361087.
Attachment #248226 - Flags: superreview?(bzbarsky)
Attachment #248226 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 7

12 years ago
Setting dependency as per comment #6
Depends on: 361087
(Assignee)

Comment 8

12 years ago
It's not dependent, I just was lazy to create a separate patch. If this end up getting reviewed sooner than the patch in bug 361087, I'll post a patch for check-in.
Comment on attachment 248226 [details] [diff] [review]
patch [not for checkin]

>Index: content/base/public/nsIStyleSheetLinkingElement.h
>+   * Tells this element to use a different base URI. This is used for
>+   * proper loading of xml-stylesheet processing instructions in XUL overlays
>+   * and is only currently used by nsXMLStylesheetPI.

Please add asserts in the other subclasses as needed?  That is, we want to assert at some point if someone calls this on a subclass that doesn't implement it.  Perhaps just assert in nsStyleLinkElement and put the member and the working accessor in nsXMLStylesheetPI?

Or you could just make this work for html:link/style too.  Shouldn't be too bad.

>+   * @param newBaseURI the new base URI, NULL to use the default base 

"nsnull", not "NULL".  And "aNewBaseURI" throughout this patch.

You need to change the IID of this interface.

The rest looks good.  r+sr=bzbarsky with those nits picked and asserts added.
Attachment #248226 - Flags: superreview?(bzbarsky)
Attachment #248226 - Flags: superreview+
Attachment #248226 - Flags: review?(bzbarsky)
Attachment #248226 - Flags: review+
(Assignee)

Comment 10

12 years ago
Created attachment 248909 [details] [diff] [review]
updated patch

Review comments fixed. I'll write a sticky note about not forgetting to update an interface's uuid.
Attachment #248226 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
> Or you could just make this work for html:link/style too.  Shouldn't be too
> bad.

I'd rather not add this magical behavior to a random subset of elements, especially since the author will have no way to override the base URI for such elements with the approach I've taken.

We ought to figure out the story with base URI in overlays (do we want the base URI be the overlay's URI in all cases?).
No longer depends on: 361087
Whiteboard: [checkin needed]
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed] → [checkin needed - attachment 1 and 3]
> +nsStyleLinkElement::OverrideBaseURI(nsIURI* newBaseURI)

aNewBaseURI?  I'll make that change before checking in, I guess.
Attachment #248224 - Attachment mime type: application/octet-stream → application/zip
Created attachment 248959 [details] [diff] [review]
Patch that I checked in
Attachment #248224 - Attachment is obsolete: true
Attachment #248909 - Attachment is obsolete: true
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
I had to check in a bustage fix, because there was an nsCOMPtr on the left of the ':' in a ?: and a raw ptr on the right.  Just added a .get() to the COMPtr.
(Assignee)

Comment 16

12 years ago
Thanks for the check-in, Boris, and sorry about the bustage and the forgotten parameter.
Whiteboard: [checkin needed - attachment 1 and 3]
(Assignee)

Updated

12 years ago
Flags: in-testsuite+
Keywords: testcase
Flags: blocking1.9?
(Assignee)

Updated

12 years ago
Target Milestone: --- → mozilla1.9alpha2
You need to log in before you can comment on or make changes to this bug.