Closed Bug 1037687 Opened 6 years ago Closed 5 years ago

Investigate if iframe/frame/object in shadow dom hosted by in-tree element should be loaded

Categories

(Core :: DOM: Core & HTML, defect)

29 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

So far I haven't found any spec defining they should load, but that is probably
a bug in HTML or DOM or Shadow DOM spec.
We'd like loading iframes if possible. How hard is this to implement?
Flags: needinfo?(bugs)
Making iframe to load should be close to trivial, but it isn't clear to me how session history should work.
What kind of behavior would gaia prefer for session history? Should the pages inside shadow iframe participate session history?
Flags: needinfo?(bugs)
Assignee: nobody → bugs
Comment on attachment 8567157 [details] [diff] [review]
v1

Review of attachment 8567157 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameLoader.cpp
@@ +1191,5 @@
>      otherChildDocument->GetParentDocument();
>  
>    // Make sure to swap docshells between the two frames.
> +  nsIDocument* ourDoc = ourContent->GetComposedDoc();
> +  nsIDocument* otherDoc = otherContent->GetComposedDoc();

I think we also want to do replace GetCurrentDoc() with GetComposedDoc() in nsFrameLoader::SwapWithOtherRemoteLoader
Attachment #8567157 - Flags: review?(wchen) → review+
ah yes.

(I was thinking whether to change swap implementations at all.)
Attached patch v2Splinter Review
Ok, that particular case was easy, and defined in the spec.

filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=28086 for a related case which is not defined in the spec.
Attachment #8568260 - Flags: review?(wchen)
Look for ShouldExposeChildWindow in the v2
Comment on attachment 8568260 [details] [diff] [review]
v2

Review of attachment 8568260 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/WindowNamedPropertiesHandler.cpp
@@ +21,5 @@
>  {
> +  nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aChild);
> +  NS_ENSURE_TRUE(piWin, false);
> +  Element* e = piWin->GetFrameElementInternal();
> +  if (e && e->IsInShadowTree()) {

If we want to match the spec language here (which uses "in a document") [1], we should use !e->IsInUncomposedDoc(). Although if you do decide to change it, it should also include a comment mentioning that the check is to ensure shadow DOM content is not exposed.

The shadow DOM spec also mentions named properties but it doesn't actually say anything useful: "Window object named properties [HTML] must access the nodes in the document tree." (doesn't say that it should not access nodes in a shadow tree)

[1] https://html.spec.whatwg.org/multipage/browsers.html#child-browsing-context
Attachment #8568260 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #9)
> ::: dom/base/WindowNamedPropertiesHandler.cpp
> @@ +21,5 @@
> >  {
> > +  nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aChild);
> > +  NS_ENSURE_TRUE(piWin, false);
> > +  Element* e = piWin->GetFrameElementInternal();
> > +  if (e && e->IsInShadowTree()) {
> 
> If we want to match the spec language here (which uses "in a document") [1],
> we should use !e->IsInUncomposedDoc(). Although if you do decide to change
> it, it should also include a comment mentioning that the check is to ensure
> shadow DOM content is not exposed.
Well, you don't get here if you're dealing with nodes not in composed doc.
So in practice IsInShadowTree and !IsInUncomposedDoc() should have the same effect.
not sure which one is more understandable.
https://hg.mozilla.org/mozilla-central/rev/53365d1c2b7f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Wow thanks guys, you're machine-like :)
You need to log in before you can comment on or make changes to this bug.