Closed
Bug 532569
Opened 15 years ago
Closed 15 years ago
integrate iframe into chrome view hierarchy when specified by XUL attribute
Categories
(Core :: Web Painting, defect, P2)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
People
(Reporter: myk, Assigned: roc)
References
Details
Attachments
(1 file)
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
bzbarsky
:
superreview+
beltzner
:
approval1.9.2.2+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•15 years ago
|
blocking2.0: ? → beta1
Priority: -- → P2
Assignee | ||
Comment 1•15 years ago
|
||
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)
Comment 2•15 years ago
|
||
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?
Assignee | ||
Comment 3•15 years ago
|
||
(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 4•15 years ago
|
||
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?
Assignee | ||
Comment 5•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #425932 -
Flags: superreview?(bzbarsky) → superreview+
Comment 6•15 years ago
|
||
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?
Updated•15 years ago
|
Attachment #425932 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 7•15 years ago
|
||
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.
Comment 8•15 years ago
|
||
Sold!
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs landing]
Reporter | ||
Comment 9•15 years ago
|
||
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
Reporter | ||
Updated•15 years ago
|
Target Milestone: mozilla1.9.3a2 → ---
Assignee | ||
Comment 10•15 years ago
|
||
yay!
Reporter | ||
Comment 11•15 years ago
|
||
The Mac build tests similarly came up roses. The Windows build will take a bit longer.
Reporter | ||
Comment 13•15 years ago
|
||
The Windows build seems to work as well!
Assignee | ||
Comment 14•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Assignee | ||
Comment 15•15 years ago
|
||
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 16•15 years ago
|
||
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+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs 192 landing]
Reporter | ||
Comment 17•15 years ago
|
||
Landed on 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b54d9f55473a
status1.9.2:
--- → .2-fixed
Whiteboard: [needs 192 landing]
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•