Closed Bug 579349 Opened 12 years ago Closed 12 years ago
Fennec: Content Window pops up over url bar when sidebars are visible
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
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/
Another use case is the new Find feature. It's hidden unless you pan down a bit.
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.
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.
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.
(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.)
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
(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.
(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;".
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.
This will make a fine reftest
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.
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.
Attachment #461142 - Flags: review?(tnikkel)
I verified that this patch fixes Fennec.
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".
Attachment #461142 - Flags: review?(tnikkel) → review+
(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.
Whiteboard: [needs landing]
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
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.
In BasicLayerManager::EndTransaction, we should be taking the useDoubleBuffering path in that case. Are we?
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.
(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?
Right away matt. I was just taking precautions to avoid duping
You need to log in before you can comment on or make changes to this bug.