Closed Bug 363406 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha2

People

(Reporter: philip.chee, Assigned: asqueella)

References

()

Details

(Keywords: regression, testcase)

Attachments

(1 file, 3 obsolete files)

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.
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
Blocks: 319654
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?
Not being able to generate patches for new subdirectories sucks.
Attached patch patch [not for checkin] (obsolete) — Splinter Review
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)
Status: NEW → ASSIGNED
Setting dependency as per comment #6
Depends on: 361087
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+
Attached patch updated patch (obsolete) — Splinter Review
Review comments fixed. I'll write a sticky note about not forgetting to update an interface's uuid.
Attachment #248226 - Attachment is obsolete: true
> 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]
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
Attachment #248224 - Attachment is obsolete: true
Attachment #248909 - Attachment is obsolete: true
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 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.
Thanks for the check-in, Boris, and sorry about the bustage and the forgotten parameter.
Whiteboard: [checkin needed - attachment 1 and 3]
Flags: in-testsuite+
Keywords: testcase
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: