Closed
Bug 1081677
Opened 10 years ago
Closed 10 years ago
We can have several DOMContentLoaded events with HTML Imports
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: julienw, Assigned: gkrizsanits)
References
Details
Attachments
(1 file)
1.69 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
It seems we get a DOMContentLoaded event on window when the linked file is imported (which means we can get several DOMContentLoaded events which is IMO dangerous for the web). This happens if the first DOMContentLoaded was already sent before the linked file is imported. This can happen if there are a lot of script/css files to be loaded before the imported file. I managed to reproduce by simulating this behavior, inserting the link after the frist DOMContentLoaded. See http://everlong.org/mozilla/testcase-import/ I don't really know which pref I need to make rel=import work; this doesn't work on a fresh profile but works in a DEBUG Gaia profile. This happens when I run the Gaia::SMS application inside Firefox (I can give more information in how to do this if you want) because we use `rel="import"` links for some time, with some JS to simulate what was implemented in bug 877072.
Comment 1•10 years ago
|
||
We certainly shouldn't be firing DOMContentLoaded on the window from import documents... Why are we?
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1) > We certainly shouldn't be firing DOMContentLoaded on the window from import > documents... Why are we? All the import documents share the same window, so I guess this is a side effect of that fact... and we should guard against this somehow. My problem is that I do depend on this event internally in the ImportLoader, so is there a way to prevent this event to bubble up further from the import document itself? I would just call naively stopPropagation on it from http://mxr.mozilla.org/mozilla-central/source/content/base/src/ImportManager.cpp#555 Does that sound like a sane idea? The other problem is ofc then we might have the same issue for other events too, so maybe we should find a more generic way to prevent events of imported documents to bubble up further than the document itself...
Flags: needinfo?(gkrizsanits)
Comment 3•10 years ago
|
||
Share the same window? In which way? Does imported document's .defaultView() return the Window object? If you want to prevent events to not propagate from Document higher up, you want to change nsDocument::PreHandleEvent. (but it still sounds weird that imported documents share the Window, does the spec really define it that way? and if so, why wouldn't event propagate to Window?)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #3) > Share the same window? In which way? > Does imported document's .defaultView() return the Window object? According to the spec html imports do not have their own browser context, but window returns the window of the master and scripts in imported documents running in the context of the window, but defaultView is null (I have not yet landed that patch). So I would say, in a weird way...
Comment 5•10 years ago
|
||
I don't see http://w3c.github.io/webcomponents/spec/imports/ mentioning 'window' anywhere. Running scripts in the context of the main document's window doesn't really mean "sharing" to me. That is closer to what normal data documents do, but we'd just enable script execution by using something like if (IsImportDoc() && (nsIScriptGlobalObject* sgo = GetScopeObject())) { ...use sgo to as the scope. }
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5) > I don't see http://w3c.github.io/webcomponents/spec/imports/ mentioning > 'window' anywhere. Yeah, it's not over specified at the moment... https://www.w3.org/Bugs/Public/show_bug.cgi?id=26682 > > Running scripts in the context of the main document's window doesn't really > mean "sharing" to me. Probably "sharing" was a poor choice of word, it's certainly very vague and probably misleading. > That is closer to what normal data documents do, but we'd just enable script > execution by using Exactly. > something like > if (IsImportDoc() && (nsIScriptGlobalObject* sgo = GetScopeObject())) { > ...use sgo to as the scope. > } What I did is changing getWindowInternal to return the window of the master for imports http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#4709, that might have been a bad move... Maybe we should just do something less generic like what you suggest.
Assignee | ||
Comment 7•10 years ago
|
||
In particular the main reason I did return window from GetWindowInternal was this: 1. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#295 but also various stuff in nsContentUtils like: 2. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3687 For 1., I could instead return null from GetWindowInternal for imports and do the same logic logally instead. What should I do with 2. ? AdoptNode is also an interesting case... mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7489
Comment 8•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7) > In particular the main reason I did return window from GetWindowInternal > was this: > 1. > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsScriptLoader.cpp#295 That should explicitly check whether we have a window or are in import document (or nsIDocument could perhaps have some helper method of in-browsing-context-or-import-document) > 2. > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > nsContentUtils.cpp#3687 That is not supposed to work for import documents. Why would one use an import document or element in it to dispatch events to chrome?
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #8) > > 2. > > http://mxr.mozilla.org/mozilla-central/source/content/base/src/ > > nsContentUtils.cpp#3687 > That is not supposed to work for import documents. Why would one use an > import document or element in it to dispatch events to chrome? Not sure yet. I can imagine more and more XBL stuff to be replaced by web components in the future. In particular I can imagine chrome only components that are used internally that might need this. But for now I think we're safe to turn it off, and get back to it once we have an actual use case for it.
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f4662d9027be
Assignee: nobody → gkrizsanits
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8504624 -
Flags: review?(bugs)
Comment 12•10 years ago
|
||
Comment on attachment 8504624 [details] [diff] [review] returning null from GetWindowInternall for imports Oh, MasterDocument() returns something even in case of data documents. The comment in nsIDocument is just a bit wrong, since there might not be any browsing context. Couldn't you just always do nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mDocument->MasterDocument()->GetWindow()); No need for the if. If we don't have master document, MasterDocument() returns just the document itself. And in case of data documents GetWindow() will return null. With that, r+ (Not related to this bug, but do import documents inherit loadgroup from the master? The code above takes loadgroup from document, not from master, so just wondering whether that works.)
Attachment #8504624 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #12) > No need for the if. Ah yeah, indeed, thanks for noticing it. > > > (Not related to this bug, but do import documents inherit loadgroup from the > master? The code above takes loadgroup from document, not from master, so > just wondering whether that works.) We create a new loadgorup for each import in the masters loadgroup. But I'm really glad you asked it because I checked the code and instead of adding the new loudgroup to the masters I create it accidentally in the import parents loadgroup which is a bug.
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e87e494687
https://hg.mozilla.org/mozilla-central/rev/c7e87e494687
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•