Closed Bug 885289 Opened 7 years ago Closed 7 years ago

Improve context menu behaviour for srcdoc iframes


(Firefox :: Menus, defect)

Not set



Firefox 25


(Reporter: jkitch, Assigned: jkitch)




(1 file, 1 obsolete file)

Bug 802895 disables the "This Frame" context menu item for srcdoc iframes.  This was done to prevent "show only this frame" etc. displaying arbitrary web content in the about protocol that was previously reserved for browser-internal/addon pages

A consequence of this is that it will hide the "view image"/"view video" context menu items for images/videos in srcdoc frames, which probably isn't optimal.

To fix that we'd probably need to track inFrame and inSrcdocFrame separately, so that srcdoc iframes retain a "this frame" sub-menu, but without the offending menu options.
Depends on: 802895
Assignee: nobody → jkitch.bug
Attached patch menu (obsolete) — Splinter Review
This hides the frame entries that are undesirable or do not make sense for srcdoc iframes.

The question is, whether suppressing the ability to open srcdoc iframes in isolation is still undesirable.

Since the decision to suppress them was made, new identity elements have been introduced to indicate secure Firefox-internal pages.  If the srcdoc document in isolation displayed the standard grey globe icon (with an about:srcdoc URI), would it be clear that it is untrusted, arbitrary content?
Attachment #778431 - Flags: review?(
Comment on attachment 778431 [details] [diff] [review]

Someone else (bz?) should review the nsContentUtils change. At first glance it seems odd to be special-casing those particular schemes (perhaps it should be a check for nested URIs in general, using the innermost if present?).

I'll delegate the nsContextMenu reviews to mconley.
Attachment #778431 - Flags: review?( → review?(mconley)
Comment on attachment 778431 [details] [diff] [review]

Requesting review for the nsContentUtils portion of the patch.

The check is necessary to allow viewing the source of a srcdoc document to work in debug builds.
Attachment #778431 - Flags: review?(bzbarsky)
Comment on attachment 778431 [details] [diff] [review]

r=me on the contentutils change.
Attachment #778431 - Flags: review?(bzbarsky) → review+
Comment on attachment 778431 [details] [diff] [review]

Review of attachment 778431 [details] [diff] [review]:

r=me with my concerns resolved. Thanks!

::: browser/base/content/test/test_contextmenu.html
@@ +1060,5 @@
>      select_inputtext = subwindow.document.getElementById("test-select-input-text");
>      select_inputtext_password = subwindow.document.getElementById("test-select-input-text-type-password");
>      plugin = subwindow.document.getElementById("test-plugin");
>      longdesc = subwindow.document.getElementById("test-longdesc");
> +    srcdoc = subwindow.document.getElementById("test-srcdoc");

Unless I'm mistaken, srcdoc should be added to the list of variables here:

::: content/base/src/nsContentUtils.cpp
@@ +6105,5 @@
>      // srcdoc loads, but the URI is non-resolvable in cases where it is not).
>      if (aForceOwner) {
>        nsAutoCString uriStr;
>        aURI->GetSpec(uriStr);
> +      if(!uriStr.EqualsLiteral("about:srcdoc") && 

Trailing whitespace.
Attachment #778431 - Flags: review?(mconley) → review+
Attached patch patchSplinter Review
Changes applied
Attachment #778431 - Attachment is obsolete: true
Attachment #779686 - Flags: checkin?
Comment on attachment 779686 [details] [diff] [review]

You can just use the checkin-needed bug keyword in the future :)
Attachment #779686 - Flags: checkin? → checkin+
False alarm, these were due to bug 890690 which has now been backed out.

This can reland - sorry for the churn!
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Doesn't seem to have an impact for developers, no doc needed. Reset the keyword with an explanation if you disagree.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.