Last Comment Bug 699351 - Ability to render browser contents outside a viewport
: Ability to render browser contents outside a viewport
Status: RESOLVED FIXED
: dev-doc-needed, mobile
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 708062 737784 739398 739423
Blocks: native_droid_panning 699262 702407
  Show dependency treegraph
 
Reported: 2011-11-03 02:21 PDT by Chris Lord [:cwiiis]
Modified: 2012-03-26 21:31 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dispaly-port test (6.74 KB, application/java-archive)
2011-11-03 05:44 PDT, Chris Lord [:cwiiis]
no flags Details
Enable display-port for sub-document frames (4.29 KB, patch)
2011-11-03 06:27 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Add frameLoader.clipDocument API (33.22 KB, patch)
2011-11-06 20:57 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
add a test that dynamic updates to the child's overflow area work (32.97 KB, text/plain)
2011-11-06 21:51 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
updated (41.28 KB, patch)
2011-11-07 21:14 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Screenshot of test 1 (6.31 KB, image/png)
2011-11-08 08:24 PST, Chris Lord [:cwiiis]
no flags Details
Screenshot of test 2 (5.56 KB, image/png)
2011-11-08 08:24 PST, Chris Lord [:cwiiis]
no flags Details
updated test_no_clip_iframe_2 (43.48 KB, patch)
2011-11-08 12:59 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Test xul app that fails (2.62 KB, application/x-zip)
2011-11-08 15:51 PST, Chris Lord [:cwiiis]
no flags Details
updated again (49.90 KB, patch)
2011-11-08 18:31 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
patch for review (47.38 KB, patch)
2011-11-10 00:53 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bzbarsky: superreview+
Details | Diff | Splinter Review
patch v2 (48.64 KB, patch)
2011-11-30 17:20 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 2: fix assertions (2.45 KB, patch)
2011-12-01 14:42 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 3: fix subdocument clipping (2.28 KB, patch)
2011-12-01 14:43 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 4: fix test (1.70 KB, patch)
2011-12-01 14:44 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2011-11-03 02:21:58 PDT
I would expect to be able to set a display-port on a sub-document frame so that it can render outside of its bounds, but doing so currently has no effect at all.

Example test-cases and WIP patches incoming...
Comment 1 Chris Lord [:cwiiis] 2011-11-03 05:44:14 PDT
Created attachment 571611 [details]
Dispaly-port test

This zip contains a test and a reference for how I would expect display-port to work (at least approximately). Currently, setting a displayport on a subdocument frame does nothing.

Patch incoming that makes it half-work...
Comment 2 Chris Lord [:cwiiis] 2011-11-03 06:27:12 PDT
Created attachment 571622 [details] [diff] [review]
Enable display-port for sub-document frames

This patch changes clipping so that sub-document frames with display ports set don't clip around the viewport (and instead clip around the display-port).

Unfortunately, the displayport x,y appear to be ignored, so the rendering is not offset correctly in the test.
Comment 3 Chris Lord [:cwiiis] 2011-11-03 12:56:32 PDT
I've been messing around with adding a translation in FrameLayerBuilder::DrawThebesLayer that depends on the root scroll frame of the presshell the DisplayListItem's frame is in, but results tend to be pretty inconsistent... The part of the previous patch that forces scroll-frames with display ports to get their own layer is unnecessary, but perhaps a solution will rely on it...
Comment 4 Timothy Nikkel (:tnikkel) 2011-11-03 13:13:25 PDT
If you only change the clipping then the subdocument is going to draw at the coordinates it wants to draw at. You might have to implement a new display item type, like nsDisplayRemote that draws the subdocument to a layer at the right offset (minus the inter-process parts).
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-03 17:25:33 PDT
Test URL: jar:https://bug699351.bugzilla.mozilla.org/attachment.cgi?id=571611!/test-displayport/
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-03 17:31:10 PDT
I don't really understand the problem here. Viewport clipping for the subdocument (or clipping in the parent document) should not affect displayport layer rendering for content inside the subdocument's viewport.

Chris, can you explain what you found in your debugging?
Comment 7 Chris Lord [:cwiiis] 2011-11-04 05:45:10 PDT
(In reply to Timothy Nikkel (:tn) from comment #4)
> If you only change the clipping then the subdocument is going to draw at the
> coordinates it wants to draw at. You might have to implement a new display
> item type, like nsDisplayRemote that draws the subdocument to a layer at the
> right offset (minus the inter-process parts).

The more I look into it, the more this looks like the way to go - when a displayport is encountered, build an nsDisplayPortItem (or something) that has altered GetBounds/GetVisibility depending on the display-port perhaps?

How would you go about implementing this though? Would it inherit from nsDisplayWrapList or something like that? Or is that unnecessary? This code is pretty hairy and it's taken me a very long time to even get to the basic level of understanding now - any pointers would really be appreciated!

Would you be able to guide me through writing this?


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> I don't really understand the problem here. Viewport clipping for the
> subdocument (or clipping in the parent document) should not affect
> displayport layer rendering for content inside the subdocument's viewport.
> 
> Chris, can you explain what you found in your debugging?

The real problem is we want to have a browser element that's sized at some large size, say 1000x1000, but behaves as if it's actually 800x600 with respect to layout, and has a movable render origin (but always filling out the larger size). We need this to do asynchronous scrolling in the native Android Fennec.

This is currently done in XUL e10s fennec by having a small-sized browser and setting a large display port on it, but this code path basically only works in the very specific way in which e10s fennec uses it.

We don't so much care about the individual layer contents atm, so much as the final rendered output - and that output needs to do what I described a couple of paragraphs above.

It's easy enough to alter clipping when a frame has a display-port set, but getting the render offset to happen and behave correctly is proving to be difficult.
Comment 8 Patrick Walton (:pcwalton) 2011-11-04 08:48:29 PDT
(In reply to Chris Lord [:cwiiis] from comment #7)
> The real problem is we want to have a browser element that's sized at some
> large size, say 1000x1000, but behaves as if it's actually 800x600 with
> respect to layout, and has a movable render origin (but always filling out
> the larger size). We need this to do asynchronous scrolling in the native
> Android Fennec.

Note that what I'm doing right now (as of last night) is to display the browser at 1024x2048 (the display port size) and to have chrome monkey-patch the CSS of the content document so that the <html> element ends up with a width of 980 and a height of (980 * aspect ratio). This is obviously not something we can ship. The goal is to achieve this same effect, but without monkey patching the content CSS.

(setCSSViewport() almost works, but doesn't allow rendering outside the CSS viewport for most pages, making it useless.)
Comment 9 Patrick Walton (:pcwalton) 2011-11-04 09:05:18 PDT
(In reply to Chris Lord [:cwiiis] from comment #7)
> It's easy enough to alter clipping when a frame has a display-port set, but
> getting the render offset to happen and behave correctly is proving to be
> difficult.

Does scrollTo() work? If you've got the clipping rect working, and scrollTo() works, then I could just use that and it'd be better than monkey patching content CSS...
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-05 02:37:50 PDT
In e10s Fennec, RenderFrameParent handles panning and zooming by adding extra transforms to the shadow layer tree. So the issues you're having here are because you need the content process to handle panning for you. Have I got that right?

canvas.drawWindow can render outside the viewport by passing nsIPresShell::RenderDocument the RENDER_IGNORE_VIEWPORT_SCROLLING flag.

I assume there isn't literally a <browser> element, since there's no XUL in this model. Is that right?

Thanks for https://bugzilla.mozilla.org/show_bug.cgi?id=695448#c1.
How does the control flow work when you want the Gecko thread to paint into its buffer?
Comment 11 Patrick Walton (:pcwalton) 2011-11-05 10:23:22 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I assume there isn't literally a <browser> element, since there's no XUL in
> this model. Is that right?

There actually is a <browser> element (inside a deck). "XUL-less Fennec" is kind of inaccurate -- it's more like minimum-XUL Fennec.

> How does the control flow work when you want the Gecko thread to paint into its buffer?

1. Before a paint, Java has to make sure that the browser viewport is correct on the Gecko side. If Java needs to move the browser viewport around, it first sends a custom "PanZoom:PanZoom" event. browser.js catches this and moves the viewport (via scrollTo() at the moment -- zoom doesn't work yet but could potentially be faked with CSS transforms or canvas). After the movement occurs, Gecko sends a "PanZoom:Ack". Java receives this, stores the new viewport rect, and proceeds.

2. Java sends a special Android "DRAW" event. The widget code receives this event and notifies Java (via JNI) that it's planning to draw (via GeckoSoftwareLayerClient.beginDrawing()). It receives the buffer pixels again via JNI, with GeckoSoftwareLayerClient.lockBuffer(). (This locks on the Java side so that the compositor won't try to upload a texture while Gecko is drawing to it.)

3. Widget sets up a surface using the buffer that it received and renders all of the nsWindows it knows about into it. (There are typically only two in Native UI Fennec, one for the browser and one for the content.)

4. Once rendering is complete, widget calls unlockBuffer() and endDrawing() on the GeckoSoftwareLayerClient via JNI. The endDrawing() call simultaneously moves the tile around if necessary and schedules a texture upload on the rendering thread. Note that the call to endDrawing() can optionally contain a dirty rectangle, and if provided only the scanlines that changed will be uploaded to the texture; this is used when Gecko wants to draw on its own. (When Java initiates a draw request, however, it's typically because of a pan or a zoom, which causes the whole surface to be repainted.)

5. The next time our rendering thread wakes up to render, it notices that there's a texture upload that needs to happen and performs it. (Android has a built-in refresh driver that wakes up our rendering thread at the right intervals, so we just use that.) It then composites everything (the page itself, plus a bit of eye candy I added) to the screen.

When Gecko itself initiates a render, we start at step 2, with the difference that instead of being triggered by a DRAW event the functionality is triggered by nsWindow::Invalidate.

Hopefully this explanation is what you were after. Sorry if it's unclear.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 01:31:42 PDT
Thanks for that.

(In reply to Patrick Walton (:pcwalton) from comment #11)
> 3. Widget sets up a surface using the buffer that it received and renders
> all of the nsWindows it knows about into it. (There are typically only two
> in Native UI Fennec, one for the browser and one for the content.)

I would have expected only one native widget, for the XUL document itself.

Maybe the correct API here is an API that lets you set a transform on a <browser> or <iframe> that is applied to the document contents before viewport clipping?

I'm not sure how that transform should affect the display of viewport scrollbars. Are you rendering the content viewport's scrollbar(s) in Gecko, or on top of the content using Java?
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 01:34:13 PDT
(BTW, I don't really understand the big picture in terms of design and schedule here, but it seems to me unfortunate that your approach is intended to be a very quick stop-gap solution (right?) yet requires new rendering API from layout, after which we'll go back to using the existing (shadowlayers) API.)
Comment 14 Patrick Walton (:pcwalton) 2011-11-06 10:14:41 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> I would have expected only one native widget, for the XUL document itself.

Hmm. There are definitely two native widgets; I had assumed that Gecko was supposed to generate a new native widget for the content, but if that isn't supposed to happen I'm not sure what the second native widget is. I can investigate, but it doesn't seem critical.

> Maybe the correct API here is an API that lets you set a transform on a
> <browser> or <iframe> that is applied to the document contents before
> viewport clipping?

That would work.

> I'm not sure how that transform should affect the display of viewport
> scrollbars. Are you rendering the content viewport's scrollbar(s) in Gecko,
> or on top of the content using Java?

Our scrollbars will be rendered in Java. That way they stay accurate when panning and zooming, even if Gecko hasn't caught up.

> (BTW, I don't really understand the big picture in terms of design and schedule here,
> but it seems to me unfortunate that your approach is intended to be a very quick
> stop-gap solution (right?) yet requires new rendering API from layout, after which we'll
> go back to using the existing (shadowlayers) API.)

The options I see (and saw at the time) are:

1. Have Java sync on Gecko, rendering in one texture with paint events (the current birch approach).

2. Have Java sync on Gecko, rendering with GL layers.

3. Implement non-e10s shadow layers.

4. Use shadow layers and implement a layer manager in Java; replace all the uses of IPDL with JNI in gfx.

5. Use the "single tile managed by a Java compositor" approach being discussed here; render using something like a display port or the API you proposed in comment #12.

6. Same as (4), but use a CSS transform to implement panning and zooming.

7. Same as (4), but use a canvas and drawWindow().

Cons of each, at least as cjones and I saw it:

Option (1) is the current birch design. In some ways, this is the biggest competition to the current approach, since in many ways pan is fast enough already and it wins in memory use. However, it's a risk because of slow pan on complex sites and slow zoom on all sites (see bug 698071 and bug 699704).

Option (2) seems like a risk since we haven't enabled GL layers on mobile yet. It also shares the risk of slow pan on complex sites (it fixes the zoom issue though).

Option (3) seemed to be a severe risk to the development schedule.

Option (4) shares all of the risk of (3), but is also duplicating a bunch of code (the layer manager) in a platform-specific way.

Option (5) is unfortunate because, as you said, it adds a new API.

Option (6) struck me as likely to be slow, but maybe it's not going to be too bad. Unless there are going to be viewport issues.

Option (7) uses extra memory and I figured it would also be slow.

So these were the options I saw. I went with (5) for now, but if you have better ideas then I'm open to them. I'll probably be trying (6) soon to see if it works too.
Comment 15 Patrick Walton (:pcwalton) 2011-11-06 10:15:43 PST
(In reply to Patrick Walton (:pcwalton) from comment #14)
> 6. Same as (4), but use a CSS transform to implement panning and zooming.
> 
> 7. Same as (4), but use a canvas and drawWindow().

Er, that should be "same as (5)".
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 14:47:13 PST
(In reply to Patrick Walton (:pcwalton) from comment #14)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> > I would have expected only one native widget, for the XUL document itself.
> 
> Hmm. There are definitely two native widgets; I had assumed that Gecko was
> supposed to generate a new native widget for the content, but if that isn't
> supposed to happen I'm not sure what the second native widget is. I can
> investigate, but it doesn't seem critical.

Yeah. It's worth investigating though since there's unnecessary overhead and complexity involved with more than one widget.

> > I'm not sure how that transform should affect the display of viewport
> > scrollbars. Are you rendering the content viewport's scrollbar(s) in Gecko,
> > or on top of the content using Java?
> 
> Our scrollbars will be rendered in Java. That way they stay accurate when
> panning and zooming, even if Gecko hasn't caught up.

Good, so it doesn't matter how we define how that "viewport transform" affects the viewport scrollbars (simplest approach would be to transform them).

> 3. Implement non-e10s shadow layers.
> 
> 4. Use shadow layers and implement a layer manager in Java; replace all the
> uses of IPDL with JNI in gfx.

Do you agree that one of these is the right long-term option? I'm pretty certain (and so is cjones, judging by the "Off-main-thread compositing" thread) that shadow layers are essential in the long run.

Anyway, as for your needs here...

One way to proceed would be to add an API to nsIObjectLoadingContent that takes a transform (could just be x,y,scalex,scaley) that will be applied to subdocument content. nsSubDocumentFrame::BuildDisplayList will need to wrap an nsDisplayTransform around the childItems list before the nsDisplayClip is wrapped around it. You'd also want to override nsSubDocumentFrame::InvalidateInternal and have it transform the invalidated rect if necessary. You'd also need to change nsIFrame::IsTransformed and nsIFrame::GetTransformMatrix to account for the transform ... probably having nsViewportFrame::IsTransformed return true when the parent nsSubDocumentFrame has the special transform.

Another approach, which might be a lot simpler, is to simply add a flag to nsIObjectLoadingContent that turns off clipping on the nsSubDocumentFrame and on the nsHTMLScrollFrame that's the child of the root's nsViewportFrame. Then you could use a CSS transform on the <browser> element to do what you want. We'd need to propagate the overflow area(s) of the scrolled child frame to the nsHTMLScrollFrame and the nsViewportFrame, and have nsSubDocumentFrame pick up that overflow area.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 14:50:55 PST
I guess I'm suggesting kind-of option 6, but I don't see how you'd do option 6 without being able to turn off clipping so Gecko can draw outside the CSS viewport.

I don't see that it'll be any slower than option 5.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 14:53:46 PST
Also, FWIW, romaxa is working on something like option 4 (but not Java, some Qt thing) and has some results.
Comment 19 Patrick Walton (:pcwalton) 2011-11-06 14:56:54 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> > 3. Implement non-e10s shadow layers.
> > 
> > 4. Use shadow layers and implement a layer manager in Java; replace all the
> > uses of IPDL with JNI in gfx.
> 
> Do you agree that one of these is the right long-term option? I'm pretty
> certain (and so is cjones, judging by the "Off-main-thread compositing"
> thread) that shadow layers are essential in the long run.

Definitely. As long as we can drive it from Java appropriately (something like an nsIContentView interface would be fine), I agree. All competing browsers are moving to something like this. I don't want to keep position:fixed broken forever :) (Not to mention video performance, CSS3 transforms, etc. etc.)

> Another approach, which might be a lot simpler, is to simply add a flag to
> nsIObjectLoadingContent that turns off clipping on the nsSubDocumentFrame
> and on the nsHTMLScrollFrame that's the child of the root's nsViewportFrame.
> Then you could use a CSS transform on the <browser> element to do what you
> want. We'd need to propagate the overflow area(s) of the scrolled child
> frame to the nsHTMLScrollFrame and the nsViewportFrame, and have
> nsSubDocumentFrame pick up that overflow area.

I like this idea.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 14:59:16 PST
(In reply to Patrick Walton (:pcwalton) from comment #19)
> > Another approach, which might be a lot simpler, is to simply add a flag to
> > nsIObjectLoadingContent that turns off clipping on the nsSubDocumentFrame
> > and on the nsHTMLScrollFrame that's the child of the root's nsViewportFrame.
> > Then you could use a CSS transform on the <browser> element to do what you
> > want. We'd need to propagate the overflow area(s) of the scrolled child
> > frame to the nsHTMLScrollFrame and the nsViewportFrame, and have
> > nsSubDocumentFrame pick up that overflow area.
> 
> I like this idea.

Should I do it today then?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 15:15:35 PST
Actually the new API goes on nsIFrameLoaderOwner.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 15:19:55 PST
... or nsIFrameLoader.
Comment 23 Patrick Walton (:pcwalton) 2011-11-06 16:52:38 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Should I do it today then?

Sure, if you don't mind -- it'd be great to have this API. At your earliest convenience, of course.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 17:23:49 PST
I'm doing it now.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 20:57:56 PST
Created attachment 572383 [details] [diff] [review]
Add frameLoader.clipDocument API

Tests pass. This should be a good start at least. Let me know how it works for you.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 21:51:47 PST
Created attachment 572393 [details]
add a test that dynamic updates to the child's overflow area work
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 21:53:44 PST
Much of this code will be needed to support seamless iframes even if we don't need the clipSubdocument API later.
Comment 28 Chris Lord [:cwiiis] 2011-11-07 03:50:41 PST
This looks good - I didn't think setting the transform on the browser would work as expected, so trying this out now. I'll push results to the pan-zoom queue, thanks for dealing with this so quickly!
Comment 29 Chris Lord [:cwiiis] 2011-11-07 05:49:24 PST
Trying this out, as soon as I set clipSubdocument to false on the browser element in the attached example (and remove all display-port setting code), the whole window turns blank grey, as if nothing was inside of it - even the red background of the deck is gone.

Am I doing something wrong?
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 13:06:05 PST
I added the testcase in http://pastebin.mozilla.org/1376724 as another browser-chrome mochitest and it seems to work. Draws the 1000x1000 black rectangle.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-07 13:29:49 PST
I'm following this thread about 50%, but something to keep in mind: the nsIWindowUtils setResolution() API cooperates with scrollTo() to avoid seaming.  Need to make sure transforms work the same way, if that's used.  Does setResolution() work as intended?
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 15:10:33 PST
I don't think setResolution is needed with this approach. Just set a scale transform on the <browser> and Gecko will automatically adjust the resolution of child layers.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-07 16:54:14 PST
Guaranteed to not reflow, and without seaming when used with scrollTo()?    (ISTR your patch used common code to handle setResolution() and transforms, but just checking.)
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 17:39:58 PST
It won't reflow the <browser> contents. ThebesLayerOGL will set the PAINT_WILL_RESAMPLE flag during scaling, so disabling rotation and avoiding seaming.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 17:40:48 PST
Same on desktop BasicLayers. On mobile, the MOZ_GFX_OPTIMIZE_MOBILE flag means that seaming will be allowed.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 21:14:46 PST
Created attachment 572741 [details] [diff] [review]
updated

Fixed a couple of invalidation bugs. Content outside the viewport wasn't being invalidated correctly. Also, we need to invalidate a lot more when we scroll in such a viewport. Add tests using MozAfterPaint.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 22:36:58 PST
The _2 test in that patch is my version of the test in comment #30. Looks ok in browser-chrome tests.
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-07 22:37:34 PST
I mean chrome tests, not browser-chrome tests.
Comment 39 Chris Lord [:cwiiis] 2011-11-08 03:54:46 PST
Just noting, this patch still doesn't work on xulrunner built against mozilla-central with just this patch applied. In the test in that pastebin, you end up with a blank window. The display-list, before optimisation, is just a SolidColor.

I'm just rebuilding to be able to run mochitest (which for some reason is broken in xul-fennec) and I'll report back.
Comment 40 Chris Lord [:cwiiis] 2011-11-08 05:52:04 PST
Finally gotten a build that can run mochitests ok - both tests fail for me. This is with m-c rev 79985:6e3b8000bdfd with only the patch from this bug applied.

My .mozconfig, for reference:

OBJDIR=firefox-build-pc
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../$OBJDIR

ac_add_options --enable-application=browser
ac_add_options --enable-default-toolkit=cairo-gtk2
ac_add_options --prefix=/opt/firefox
ac_add_options --enable-debug

export MOZ_DEBUG_SYMBOLS=1
Comment 41 Chris Lord [:cwiiis] 2011-11-08 05:57:20 PST
The tests pass when running all chrome tests, but they fail when run on their own. Perhaps this gives some kind of clue?
Comment 42 Chris Lord [:cwiiis] 2011-11-08 07:46:25 PST
Sorry, on further inspection I had this wrong - all the tests pass fine, I just didn't notice the test_no_clip_frame.xul (so incorrectly tried to run the non-test-prefixed files).

Unfortunately, output is still blank. I'll see about attaching a ref-test.
Comment 43 Chris Lord [:cwiiis] 2011-11-08 08:23:18 PST
So, the first test in the patch works, but I don't see any _2 test in the patch to see what this is testing.

Here's a test:

test.html:
<html><body style="margin: 0"><div style="background-color: black; width: 1000px; height: 1000px;"></div></body></html>

test.js:
let Ci = Components.interfaces;

function onload() {
  let frame = document.getElementById("frame");
  let fl = frame.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;
  fl.clipSubdocument = false;
}

test.xul:
<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        minWidth="800" width="800" minHeight="600" height="600"
        onload="onload();" title="Test 1">

  <script type="application/javascript" src="chrome://test/content/test.js"/>

  <div id="container" xmlns="http://www.w3.org/1999/xhtml" style="width: 800px; height:600px; overflow:auto; background:red">
    <iframe style="width:100px; height: 100px;" src="test.html" id="frame"/>
  </div>

</window>

test2.xul:
<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        minWidth="800" width="800" minHeight="600" height="600"
        onload="onload();" title="Test 2">

  <script type="application/javascript" src="chrome://test/content/test.js"/>

  <deck flex="1" style="background-color: red;">
    <vbox align="start">
      <browser style="width:100px; height: 100px;" src="test.html" id="frame"/>
    </vbox>
  </deck>

</window>


Screenshots attached. Test 1 sort-of works, though there's some abnormality with regards to the margin, test 2 results in a blank window.
Comment 44 Chris Lord [:cwiiis] 2011-11-08 08:24:14 PST
Created attachment 572825 [details]
Screenshot of test 1
Comment 45 Chris Lord [:cwiiis] 2011-11-08 08:24:46 PST
Created attachment 572826 [details]
Screenshot of test 2
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-08 12:59:19 PST
Created attachment 572974 [details] [diff] [review]
updated test_no_clip_iframe_2

I put your "test 2" into test_no_clip_iframe_2.xul (the actual test code is in no_clip_iframe_2.xul). I fire up chrome tests (python runtests.py --chrome) and choose that specific test, and it creates a XUL window with the desired black rectangle. This is on Windows, with a desktop Firefox build. Trunk base revision is 9b291cc0cd86 but hopefully that doesn't matter.
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-08 13:02:43 PST
Test 1 also works. The main black rectangle top-left is at 1,1, and there is black in the top-left corner in the first row and column of pixels --- both of those effects are due to the default border on HTML iframes. It looks correct to me.
Comment 48 Chris Lord [:cwiiis] 2011-11-08 15:51:06 PST
Created attachment 573029 [details]
Test xul app that fails

This zip contains a XUL app that uses this feature in the same way we want to use it in fennec, but it fails. I'm running this with a Linux firefox build, with "$OBJDIR/dist/bin/firefox -app application.ini"

Could you confirm that this works on your build, without running it via mochitest? While tests may work fine, the behaviour this is exhibiting is definitely not correct (unless I've missed something in how this is supposed to work).
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-08 16:00:15 PST
Works fine in my build.
Comment 50 Chris Lord [:cwiiis] 2011-11-08 17:28:25 PST
Just a follow-on from IRC, it seems this bug doesn't happen on Windows, but does on Linux - I've not tested to see if it happens on Android yet. On Linux, it appears to be stuck in a constant loop of reflowing. roc is looking into it.
Comment 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-08 18:31:42 PST
Created attachment 573056 [details] [diff] [review]
updated again

Alright, this fixes the infinite-reflow issue (I was calculating the overflow-changed result of nsFrame::FinishAndStoreOverflow incorectly). Fixing that has fixed the rendering of the testcase on Linux!
Comment 52 Chris Lord [:cwiiis] 2011-11-09 01:18:31 PST
Latest iteration of the patch works fine in my test-case on Linux :)
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-10 00:53:40 PST
Created attachment 573442 [details] [diff] [review]
patch for review

I think this should be reviewed with the aim of landing on mozilla-central.

This patch creates a new API, nsIFrameLoader::clipSubdocument, which when set to false will allow the subdocument to draw outside its viewport and in general outside the bounds of the frame element. This can be used to implement various rendering policies in the mobile browser. It could potentially be used for other things. The basic infrastructure will also be needed for HTML5 <iframe seamless>. It's a simple API; the semantics are very obvious.
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2011-11-23 09:15:59 PST
Comment on attachment 573442 [details] [diff] [review]
patch for review

sr=me
Comment 55 Timothy Nikkel (:tnikkel) 2011-11-29 23:24:40 PST
Comment on attachment 573442 [details] [diff] [review]
patch for review

+nsFrameLoader::SetClipSubdocument(bool aClip)
+{
+  mClipSubdocument = aClip;
+  nsIFrame* frame = GetPrimaryFrameOfOwningContent();
+  if (frame) {
+    InvalidateFrame(frame);

InvalidateFrame doesn't invalidate thebes layer contents, is that what we want here?
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-30 17:20:03 PST
(In reply to Timothy Nikkel (:tn) from comment #55)
> Comment on attachment 573442 [details] [diff] [review] [diff] [details] [review]
> patch for review
> 
> +nsFrameLoader::SetClipSubdocument(bool aClip)
> +{
> +  mClipSubdocument = aClip;
> +  nsIFrame* frame = GetPrimaryFrameOfOwningContent();
> +  if (frame) {
> +    InvalidateFrame(frame);
> 
> InvalidateFrame doesn't invalidate thebes layer contents, is that what we
> want here?

No, it's not. I'll post an updated patch.
Comment 57 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-30 17:20:30 PST
Created attachment 578141 [details] [diff] [review]
patch v2
Comment 58 Doug Turner (:dougt) 2011-11-30 18:25:28 PST
try: https://tbpl.mozilla.org/?tree=Try&rev=396faf1a07d1
Comment 59 Brad Lassey [:blassey] (use needinfo?) 2011-11-30 21:13:48 PST
pushed to inbound and backed out due to failures https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b17ab862bbc1 and https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a6db0c6c94a3
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 03:05:02 PST
11938 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/test/chrome/test_no_clip_iframe.xul | Entire updated region is painted: 0,0,0,0
(This is the test for this bug; apparently it fails on Tinderbox :-( )

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/base/crashtests/366200-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/base/crashtests/386000-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/base/crashtests/420620-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/base/crashtests/605672-1.svg | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/smil/crashtests/615872-1.svg | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/xul/document/crashtests/386914-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/xul/document/crashtests/468211-3.xul | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/content/xul/document/crashtests/583230.xul | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/456727-2.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/editor/libeditor/html/crashtests/643786-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/base/crashtests/606432-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/base/crashtests/629908-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/generic/crashtests/536692-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/style/crashtests/472237-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/style/crashtests/472237-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/style/crashtests/580685.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/xul/base/src/crashtests/475133.html | assertion count 2 is more than expected 0 assertions

I think these are all
###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || aOverflowAreas.Overflow(otype).Contains(nsRect(nsPoint(0,0), aNewSize))', file e:/builds/moz2_slave/m-in-w32-dbg/build/layout/generic/nsFrame.cpp, line 6569

Reftests have some of those assertions too, plus
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/331809-1.html | image comparison (==)

Doesn't look too bad :-). I'll look into it tomorrow.
Comment 61 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 14:42:04 PST
Created attachment 578402 [details] [diff] [review]
Part 2: fix assertions

The viewport was sometimes not including its own bounds in its overflow area.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 14:43:33 PST
Created attachment 578404 [details] [diff] [review]
Part 3: fix subdocument clipping

The root view for the subdocument includes the root frame overflow area in its bounds. So when we need to clip the subdocument, clipping to root view bounds isn't enough. Instead, we can always use the subdocument frame's inner view bounds. This simplifies the code too.
Comment 63 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 14:44:34 PST
Created attachment 578406 [details] [diff] [review]
Part 4: fix test

The test wasn't flushing the subdocument, so we weren't picking up all the invalidations.
Comment 64 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 14:45:07 PST
Pushed to try, rev 2880a7cb5e9e
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 14:45:44 PST
(I'm hopefully that these three patches fix all the test failures)
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-01 19:35:43 PST
https://tbpl.mozilla.org/?tree=Try&rev=2880a7cb5e9e looks good.
Comment 67 Timothy Nikkel (:tnikkel) 2011-12-02 01:24:01 PST
Comment on attachment 578404 [details] [diff] [review]
Part 3: fix subdocument clipping

     // If we are going to use a displayzoom below then any items we put under
     // it need to have underlying frames from the subdocument. So we need to
     // calculate the bounds based on which frame will be the underlying frame
     // for the canvas background color item.
     nsRect bounds;
     if (subdocRootFrame) {
-      nsPoint offset = mInnerView->GetPosition() +
-                       GetOffsetToCrossDoc(aBuilder->ReferenceFrame());
-      offset = offset.ConvertAppUnits(parentAPD, subdocAPD);
-      bounds = subdocView->GetBounds() + offset;
+      bounds = subdocBoundsInParentUnits.ConvertAppUnitsRoundOut(parentAPD, subdocAPD);


Maybe I'm misunderstanding, but don't we want bounds from the root view of the subdocument here?
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-02 02:20:11 PST
I believe the subdocument root view and mInnerView have the same bounds, adjusting for appunit differences.
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-02 02:29:14 PST
Actually that's not true when the suboducment root frame has overflow. But in that case we want to use the mInnerView bounds anyway.
Comment 72 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 14:48:46 PDT
Seems to have caused a regression with Prestashop admin panels...

1) Open http://www.prestashop.com/en/demo
2) Click "View our Store Admin"
3) Enter the credentials: demo@prestashop.com / prestashop_demo
4) Click the "Catalog" tab
5) Click the "motherboard" entry from the products list
6) Scroll down to "Short Description" and try typing, copy+paste in the textbox

Nightly 2012-12-01: short description text entry works
Nightly 2012-12-06: short description text entry does not work
Comment 73 Boris Zbarsky [:bz] (still a bit busy) 2012-03-26 14:51:37 PDT
Anthony, can you please file a bug to track the regression?
Comment 74 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 14:56:22 PDT
(In reply to Boris Zbarsky (:bz) from comment #73)
> Anthony, can you please file a bug to track the regression?

Done. See bug 739423.
Comment 75 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 21:31:36 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #74)
> (In reply to Boris Zbarsky (:bz) from comment #73)
> > Anthony, can you please file a bug to track the regression?
> 
> Done. See bug 739423.

Some follow-up: it appears as though the regression I reported was fixed by bug 737784. I've tested Nightly, Aurora, and Beta from a day after landing and can no longer reproduce the issue.
(see bug 739423 comment 2)

Note You need to log in before you can comment on or make changes to this bug.