579349 - Fennec: Content Window pops up over url bar when sidebars are visible

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010716 Namoroka/4.0b2pre Fennec/2.0a1pre

Steps to Reproduce:
1. Go to addons.mozilla.org or any page that would require a vertical pan
2. Slide to the left or right to open one of the sidebars on Fennec
3. Slide up to pan down

Actual Results:
The content pans on top of the url bar

screenshot: http://www.flickr.com/photos/aakashhdesai/4799526968/

Expected Results:
The content should pan below the url bar

Comment 1

[Aakash Desai [:aakashd]]

I believe this is a more general issue where the content window is popping up over our chrome. Here's another case -

1. Slide to the left to open the controls sidebar
2. Click on the bookmarks star
3. Click on edit

Actual Results:
The edit bookmarks dialog is underneath the content window

screenshot: http://www.flickr.com/photos/aakashhdesai/4799587242/

Comment 4

[Aakash Desai [:aakashd]]

Another use case is the new Find feature. It's hidden unless you pan down a
bit.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is actually quite weird.

One thing that would help is if you can bisect the layers patches to find the exact changeset that caused the regression.

Comment 7

[Matt Brubeck (:mbrubeck)]

According to my bisecting, the first bad revision is: http://hg.mozilla.org/mozilla-central/rev/c6ecff6b8a91

That revision shows only part of the problem - the Fennec content area gets drawn on part of some of the urlbar chrome, but not all of it.  Also, there are artifacts when panning the content that do not appear in mozilla-central tip.  I'd have to do a second bisect to find where the remaining problems started, which I don't have time for just now.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The "content area" here is just the canvas elements that make up the tiles, right?

It looks like clipping is just not happening correctly. A display list dump would be useful --- set gDumpPaintList=1 and then cause the chrome to be painted.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: gDumpPaintList output

(In reply to comment #8)
> The "content area" here is just the canvas elements that make up the tiles,
> right?

Basically, yes.  It's an <html:div> that contains a single <html:canvas> where the tiles are painted.

> It looks like clipping is just not happening correctly. A display list dump
> would be useful --- set gDumpPaintList=1 and then cause the chrome to be
> painted.

See the attached file.

(In reply to comment #7)
> I'd have to do a second bisect to find where the remaining problems started,
> which I don't have time for just now.

The current form of the problem started somewhere in patches 13 through 17 from bug 564991.  (The intermediate revisions would not build on my desktop, so I was not able to bisect further.)

Comment 10

[Tony Chung [:tchung]]

This may be related side effect, but i also notice the awesomebar will flicker in the background if i slowly pan the body down the page

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #9)
> Created attachment 460749 [details]
> gDumpPaintList output

Thanks!

Do you recall what the visual problem was in that case?

Unfortunately layer clip properties didn't get dumped because we don't have code for that. I'll attach a patch for that. It also looks like the transform isn't being dumped correctly.

There are three canvases, it looks like one on the left (page thumbnails) and two canvases representing the Web content to the right.

Comment 12

[Matt Brubeck (:mbrubeck)]

(In reply to comment #11)
> Do you recall what the visual problem was in that case?

I'm pretty sure that dump happened while the window looked the same as this screenshot: http://www.flickr.com/photos/aakashhdesai/4799526968/

> There are three canvases, it looks like one on the left (page thumbnails) and
> two canvases representing the Web content to the right.

Note: One of the two canvases representing web content (#view-buffer) is hidden with style="display:none;".

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm a dope. The problem with the address bar, at least, is nothing to do with clipping; the addressbar is fixed-pos and should be over the Web content.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: HTML testcase

This will make a fine reftest

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that we build up in data->mVisibleAboveRegion information about what's visible above each ThebesLayer. We do that by adding the GetVisibleRect of each display item that's placed above the ThebesLayer. When we consider whether to add a new display item to that ThebesLayer, we check whether it intersects the mVisibleAboveRegion. However, the GetVisibleRects for the items already in the layer might have had the region of the new item excluded from them during nsDisplayItem::ComputeVisibility because they were covered by it!

I think the solution is to add a third region, say data->mContentAboveRegion, that tracks the bounds of all content placed above each ThebesLayer and check against that before we add content to the layer.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

See comments above.

In FindThebesLayerFor we compare the "draw region" above a layer L to the *visible* rect of the current item before deciding to place the current item in layer L. This is intentional. We do not need to check the full draw rect of the item, it's OK for the draw rect and L to intersect in areas that are covered by content above them both.

Also note that we're intersecting the item bounds with the cliprect to compute the "draw rects" used here.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I verified that this patch fixes Fennec.

Comment 18

[Timothy Nikkel (:tnikkel)]

Comment on attachment 461142 [details] [diff] [review]
fix

+     * @param aDrawRect the area of the item that would be drawn

That would be drawn given what conditions?

+     * The region containing the bounds of all display items in the layer.
+     * Same coordinate system as mVisibleRegion.
+     */
+    nsIntRegion  mDrawRegion;

Maybe add "whether or not they are visible" to make it clearer. Similar for the description of mContentAboveRegion.

+        // Add the entire bounds rect to the mVisibleAboveRegion.
+        // The visible region may be excluding opaque content above the
+        // item, and we need to ensure that that content is not placed
+        // in a ThebesLayer below the item!

Don't you mean mContentAboveRegion here?

I think FindThebesLayerFor should have a comment outlining what it does, ie something like "look through the thebes layer data for the first one that meets these conditions".

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #18)
> Comment on attachment 461142 [details] [diff] [review]
> fix
> 
> +     * @param aDrawRect the area of the item that would be drawn
> 
> That would be drawn given what conditions?

Drawn if we ignore visibility considerations. I'll add that.

> +     * The region containing the bounds of all display items in the layer.
> +     * Same coordinate system as mVisibleRegion.
> +     */
> +    nsIntRegion  mDrawRegion;
> 
> Maybe add "whether or not they are visible" to make it clearer. Similar for
> the description of mContentAboveRegion.

Sure.

> +        // Add the entire bounds rect to the mVisibleAboveRegion.
> +        // The visible region may be excluding opaque content above the
> +        // item, and we need to ensure that that content is not placed
> +        // in a ThebesLayer below the item!
> 
> Don't you mean mContentAboveRegion here?

Yes.

> I think FindThebesLayerFor should have a comment outlining what it does, ie
> something like "look through the thebes layer data for the first one that
> meets these conditions".

OK.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/491b28e3c108

Comment 21

[Doug Turner (:dougt)]

roc,

I no longer see the clipping issue with today's build.  However, i see quite a bit of flickering when the content area of fennec moves under where the URL bar is.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In BasicLayerManager::EndTransaction, we should be taking the useDoubleBuffering path in that case. Are we?

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Reproduces for Android on timestamp 20100811 
============================================
Behavior: 
 
Navigate bugzilla.mozilla.org. Tap on search field to
bring up soft keyboard. 
Slide the screen to trigger right sidebar(with options) 
Now pan the page down. 

Actual: Bookmark star icon will cascade over reload button
from location bar.

Comment 24

[Matt Brubeck (:mbrubeck)]

(In reply to comment #23)
> Actual: Bookmark star icon will cascade over reload button
> from location bar.

This sounds like a different bug (possibly related to bug 583128). Can you file it separately?

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Right away matt. I was just taking precautions to avoid duping