Closed Bug 777632 Opened 9 years ago Closed 9 years ago
Mods from operating on certain content
Firefox is landing some "social" work which creates a new sidebar and loads content from a "provider" in that sidebar, and we don't want PageMods to be able to operate on that content. Ideally there should be a way to express (in XUL or something else) that a particular <browser> element can't be injected with a PageMod.
Alex, can you see if there's any platform stuff we need for this?
Assignee: nobody → poirot.alex
Priority: -- → P1
I went through our page-mod source code and haven't been able to spot any (hacky) way to prevent that: https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/page-mod.js We can easily provide such internal feature, but what is your feeling about existing addons? Note that we are listening to `document-element-inserted` event and even if addon developer pass "*" as match pattern, it will only match http, https and ftp URIs. But I'd imagine that social sidebar document is loaded from a http URI? At the end, I think it would be just better if we add some code in SDK to ensure that the document is a child document from a Tab and reject everything else. (We are currently applying on document hosted in hidden window if they have http URIs ...)
So my thoughts here were that when we get 'document-element-inserted' we can use existing APIs (horrible nsIDocShell ones IIRC, but existing nonetheless) to get the <browser> or <iframe> loading the document. Then we could just check if say that element is type="content" or something, or look for an attribute that specifically stops pagemode nopagemod="true". Alex's suggestion of just only applying for tabs makes sense too though.
The sidebar in question is loaded from a http(s) URL, and is inside a browser with type="content". There is also another window we'd like to avoid the injection which is hosted in the hidden window. However, IIUC, in almost all cases for the tab, the <browser> element in question will often have type="content-primary" or type="content-targetable" (not sure of the spelling of the last one). So it might be possible to skip browsers with plain type="content" (and possibly another exception for those in the hidden window) Beyond that, I think the idea of restricting the pagemod only to pages in tabs might be the best option as we are likely to see more and more bits of the browser implemented in HTML (eg, we also have a <browser> hosted in a <panel> which I *suspect* is going to be hit by the pagemod, but I've not tested it). If we went the nopagemod="true" route, my concern is people will add, eg, a new <browser> in a panel and be blissfully unaware of the entire pageMod world, and thus not know they should add that attribute. Regardless, I completely agree the fix isn't obvious :)
Erik, Feel free to redirect this review to irakli if you do not feel confident about this review.
Attachment #647972 - Flags: review?(evold)
So in this patch I'm cleaning up page-mod a bit by removing old workaround needed for Firefox 4-. And I'm restricting page-mod to only tab document and their iframes.
+1, Alex - on the face of it this feels like the right boundary for page-mods. In particular, the new UX designs implement sidebars with web content similar to this, and it is hard to see a valid reason for 1 add-on to be able to modify another add-on's sidebar content.
chrome-mod should be the API to hack with these other things. It already allows to modify chrome document (which can't be modified by page-mod, even in its current state)
I like the idea Alex, I just want to confirm the tests pass on Fennec before pulling.
Status: NEW → ASSIGNED
Depends on: 784224
Attachment #647972 - Flags: review?(evold) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/b6329768740eb3681dec00f555115defb45473e2 Bug 777632: Apply page-mod only on (child) tab documents. r=@erikvold
9 years ago
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.11
You need to log in before you can comment on or make changes to this bug.