Closed Bug 532569 Opened 10 years ago Closed 10 years ago

integrate iframe into chrome view hierarchy when specified by XUL attribute

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+
status1.9.2 --- .2-fixed

People

(Reporter: myk, Assigned: roc)

References

Details

Attachments

(1 file)

It might be possible to satisfy the needs of some extensions that want to do chrome/content mashups and are currently blocked by bug 130078 by updating the most recent patch for that bug to the trunk and making its behavior conditional on the presence of a XUL attribute, as suggested by roc in bug 494238, comment 4.

We should try this to see if it's sufficient to support Jetpack panels (bug 494238), address Jetpack statusbarpanel background color issues (bug 499809), and resolve various other issues with other extensions (Personas, Taskfox, etc.).

In bug 130078, comment 98, roc suggests that Mats could do this.  Mats: do you have the time/inclination to take this on?
blocking2.0: ? → beta1
Priority: -- → P2
Blocks: 543856
Attached patch fixSplinter Review
This is very simple. The only real question is what we should call the attribute that lets you opt into an "integrated iframe". In this patch, I've decided to call it "transparent". The reason is that even after all iframe view manager hierarchies are hooked up to the root window, we still need some API for XUL authors to opt out of the behavior of root content iframes that forces the background of the iframe to be opaque (using the pref-based opaque default background if necessary). "transparent" seems like a good name for the attribute that does that opting-out. Therefore we might as well use it to trigger hooking up the view manager hierarchies during this temporary opt-in period.

The testcase tests that chrome content under the content iframe is visible in transparent parts of the iframe, and also that chrome content over the content iframe is visible.

Of course, since various bugs aren't fixed yet, using "transparent" in combination with certain features like zooming will go completely haywire. Don't do that (yet)!
Assignee: nobody → roc
Attachment #425932 - Flags: superreview?(bzbarsky)
Attachment #425932 - Flags: review?(matspal)
Is this same code responsible for deciding whether to create a widget for the subframe viewport?  If not, what code is, and do we need to change it?
(In reply to comment #2)
> Is this same code responsible for deciding whether to create a widget for the
> subframe viewport?

No. nsSubdocumentFrame does that.

> If not, what code is, and do we need to change it?

Not yet. With just this patch, plugins won't work in "transparent" iframes, but I think that's OK.
Comment on attachment 425932 [details] [diff] [review]
fix

Doesn't seem necessary to dump() the bits in case the test succeeds.

Can we really introduce a new HTML attribute just like that?
Shouldn't we at least prefix it with "moz" or some such?
(In reply to comment #4)
> (From update of attachment 425932 [details] [diff] [review])
> Doesn't seem necessary to dump() the bits in case the test succeeds.

Oops, that's just debugging cruft that I will take out.

> Can we really introduce a new HTML attribute just like that?
> Shouldn't we at least prefix it with "moz" or some such?

"transparent" is actually a no-op for content IFRAMEs --- they always support transparency. I could add code to FindContainerView to only support "transparent" on XUL IFRAMEs, but it wouldn't really have an effect.
Attachment #425932 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 425932 [details] [diff] [review]
fix

OK, looks fine, but let's get a followup bug on synchronizing the widget thing with this?
Attachment #425932 - Flags: review?(matspal) → review+
I don't really care about synchronizing the widget thing with this, because in bug 130078 I'm getting rid of the widget thing for good. I don't think we'll have any regressions in the meantime.
Whiteboard: [needs landing]
I built a recent pull of the Firefox 3.6/Gecko 1.9.2 branch on Linux with this patch and tested nine Jetpack prototype extensions that use content iframes in statusbarpanels, including seven Jetpack Gallery top downloads (GTranslatifier, Google It!, JetWave, Wikify, ClickToFlash, AdBuster, and Jetstatus) along with my Weather test extension and a local extension that simply displays some text.

With the patched build, the addition of the transparent attribute to the content iframes, and the removal of some partial workarounds in Jetpack's statusbarpanel implementation, the statusbar background color showed through the areas of the iframes that weren't covered by extension content.  And when I selected a persona with a statusbar background image, it too showed through those areas.

The extensions also appeared to behave as expected, including the couple that do DOM manipulation when clicked on.  Thus, based on this testing, it looks like this fix will resolve Jetpack's issues with placing content iframes on top of browser chrome just fine.

I'll also test on Mac and Windows once I have builds for them.
Target Milestone: --- → mozilla1.9.3a2
Target Milestone: mozilla1.9.3a2 → ---
The Mac build tests similarly came up roses.  The Windows build will take a bit longer.
The Windows build seems to work as well!
http://hg.mozilla.org/mozilla-central/rev/719a7c9db863
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Comment on attachment 425932 [details] [diff] [review]
fix

Jetpack people really want this for 3.6. Since it's an opt-in XUL feature, the risk is extremely low.
Attachment #425932 - Flags: approval1.9.2.2?
Comment on attachment 425932 [details] [diff] [review]
fix

a=beltzner for 1.9.2.2 - doesn't look like this causes API changes, but if I'm wrong about that, please make sure they're done so as not to break compatibility.
Attachment #425932 - Flags: approval1.9.2.2? → approval1.9.2.2+
Whiteboard: [needs 192 landing]
Landed on 1.9.2 branch:
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b54d9f55473a
Whiteboard: [needs 192 landing]
Blocks: 548416
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.