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)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha2
People
(Reporter: philip.chee, Assigned: asqueella)
References
()
Details
(Keywords: regression, testcase)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
I wrote this in: http://forums.mozillazine.org/viewtopic.php?t=498050
Assignee | ||
Comment 2•18 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 | ||
Comment 3•18 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?
Comment 4•18 years ago
|
||
Could add a method on nsIStyleLinkElement or whatever to set a base URI... and use that here.
Flags: blocking1.9?
Assignee | ||
Comment 5•18 years ago
|
||
Not being able to generate patches for new subdirectories sucks.
Assignee | ||
Comment 6•18 years ago
|
||
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•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•18 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 9•18 years ago
|
||
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•18 years ago
|
||
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•18 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•18 years ago
|
Whiteboard: [checkin needed] → [checkin needed - attachment 1 and 3]
Comment 12•18 years ago
|
||
> +nsStyleLinkElement::OverrideBaseURI(nsIURI* newBaseURI)
aNewBaseURI? I'll make that change before checking in, I guess.
Updated•18 years ago
|
Attachment #248224 -
Attachment mime type: application/octet-stream → application/zip
Comment 13•18 years ago
|
||
Attachment #248224 -
Attachment is obsolete: true
Attachment #248909 -
Attachment is obsolete: true
Comment 14•18 years ago
|
||
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 15•18 years ago
|
||
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•18 years ago
|
||
Thanks for the check-in, Boris, and sorry about the bustage and the forgotten parameter.
Whiteboard: [checkin needed - attachment 1 and 3]
Updated•18 years ago
|
Flags: blocking1.9?
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9alpha2
You need to log in
before you can comment on or make changes to this bug.
Description
•