Closed
Bug 1027299
Opened 9 years ago
Closed 9 years ago
Print a deprecation warning when importing XUL into content documents
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: billm, Assigned: billm)
References
Details
(Keywords: addon-compat)
Attachments
(1 file, 1 obsolete file)
9.45 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
We no longer support adding XUL nodes to content documents. However, add-ons can still create XUL nodes in a chrome document and then use contentDocument.importNode to pull them into content. It's very difficult to make this sort of thing work in electrolysis. Jorge, do you think it would be acceptable if we printed a warning that this functionality will go away in a few versions and then eventually removed it? The case I've seen is in Video Download Helper, where they create a XUL pop-up menu next to the title of videos. It seems like they could use an HTML popup just as easily. I imagine other add-ons do the same thing though. A more conservative path would be to try to use telemetry to count how often this happens (and perhaps figure out which add-ons do it). Bobby, can you think of any other paths besides importNode where XUL nodes could end up in content documents?
Flags: needinfo?(jorge)
Flags: needinfo?(bobbyholley)
Assignee | ||
Updated•9 years ago
|
Blocks: e10s-addons
Comment 1•9 years ago
|
||
We should do the same for adoptNode. I believe that adoptNode, importNode, createElement*, and parser-driven stuff should cover the cases here. Olli, can you confirm?
Flags: needinfo?(bobbyholley) → needinfo?(bugs)
Comment 2•9 years ago
|
||
Oh, there's also the case of a anonymous content from an in-content XBL binding - the videocontrols binding does this, for example.
Comment 3•9 years ago
|
||
createElement and parser driven stuff just shouldn't just work atm http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#297 XBL should end up using nsXBLBinding::GenerateAnonymousContent() which calls nsNodeUtils::Clone, so we could add some param there to suppress the warning... Or would it be enough to add a check to XULElement::BindToTree. If 'this' isn't in anonymous content, and OwnerDoc()->AllowXULXBL() returns false, print the warning (asynchronously, since doing that inside BindToTree wouldn't be safe).
Flags: needinfo?(bugs)
Comment 4•9 years ago
|
||
I'm in favor of the warning for a few versions and then breaking compatibility. I'm also CCing mig, since VideoDownload Helper is a fairly major add-on and I want him to have a good head start.
Flags: needinfo?(jorge)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → wmccloskey
Assignee | ||
Comment 5•9 years ago
|
||
I tried the BindToTree approach smaug suggested and it seems to work.
Attachment #8457729 -
Flags: review?(bugs)
Comment 6•9 years ago
|
||
Comment on attachment 8457729 [details] [diff] [review] xul-warning I think we should do this only once per document. That limits the number of warnings a bit. So add the thing to nsDeprecatedOperationList and use WarnOnceAbout, and the runnable shouldn't be posted if we have warned already so that we don't get the performance hit so often (in case some addon is doing something weird).
Attachment #8457729 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 7•9 years ago
|
||
This should do it.
Attachment #8457729 -
Attachment is obsolete: true
Attachment #8458062 -
Flags: review?(bugs)
Comment 8•9 years ago
|
||
Comment on attachment 8458062 [details] [diff] [review] xul-warning v2 >+ NS_IMETHOD Run() { nit, { goes to its own line >+ var browser = document.getElementById("browserelt"); >+ browser.addEventListener("load", function() { >+ // We add some XUL to a content document. This should generate a warning. >+ var elt = document.createElement("label"); >+ var doc = browser.contentDocument; >+ var newElt = doc.importNode(elt, false); >+ expectWarning(true, "appending XUL", function() { >+ doc.documentElement.appendChild(newElt); >+ }); >+ >+ // We add a <video> element, which contains anonymous XUL content. This should not warn. >+ var video = doc.createElement("video") >+ expectWarning(false, "appending video", function() { >+ doc.documentElement.appendChild(video); I think you should flush layout here (doc.documentElement.offsetLeft or some such) to force video to have the UI and anonymous content >+ }); Now that we warn only once, you should first check <video> and then <xul:label>
Attachment #8458062 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9e86108cf9
https://hg.mozilla.org/mozilla-central/rev/be9e86108cf9
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•