651468 - Panning through the add-ons manager doesn't stop at the end of the screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Anna (Waverley)]

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110420 Firefox/6.0a1 Fennec /6.0a1 

Device: HTC Desire Z (Android 2.2)

Steps to Reproduce:
1. Load Fennec.
2. Slide to the left to open the controls sidebar
3. Tap on the browser controls button
4. Tap on the Add-ons manager button
5. Pan down till the end of the screen (till you see the "Browse Site" and "See More Add-ons" buttons)

Actual Results:
Panning through the add-ons manager doesn't stop at the end of the screen. Scrolling continues with a blank display. Please see the video: http://www.youtube.com/user/qaioana#p/u/0/pMxpPRBK4o0

Expected Results:
Panning through the add-ons manager should stop at the end of the screen.

Comment 1

[Tony Chung [:tchung]]

regression

Comment 2

[Tony Chung [:tchung]]

Works 4/18, broken 4/19.

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2e5e4197b9db&tochange=bffd5f9dae2f

Comment 3

[Aakash Desai [:aakashd]]

The regression window was found in the previous comment; taking out the regressionwindow-wanted keyword

Comment 4

[Matt Brubeck (:mbrubeck)]

I don't see an obvious regression candidate; I'll start bisecting within the regression range from comment 2.

Comment 5

[Matt Brubeck (:mbrubeck)]

The first bad revision is http://hg.mozilla.org/mozilla-central/rev/90e46b3e8e6f

changeset:   71013:90e46b3e8e6f
user:        Robert O'Callahan <[e-mail]>
date:        Tue Apr 19 15:07:23 2011 +1200
summary:     Bug 641426. Part 5: Avoid operator== where possible to distinguish between 'equal edges' and 'equal areas' for rectangles. r=dbaron,sr=cjones

Comment 6

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

There's probably an element in your XUL with zero width but some height which didn't used to be counted as part of the scrollable area, but now (correctly) is, making the scrollable height larger than it should be.

Comment 7

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

... or something like that.

Comment 8

[Matt Brubeck (:mbrubeck)]

The extra height appears only after the add-ons are loaded, and inserted as richlistitems into the add-on list.

These richlistitems use XBL bindings from mobile/chrome/content/binding/extensions.xml.  The bindings contain anonymous content with class="show-on-select" which is set to "visibility:collapse" in mobile/themes/core/platform.css.

If I comment out the "visibility:collapse" then the content is all visible and matches the height of the container.  So it looks like the collapsed content is being included for some reason when the container height is calculated.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

At one time, I thought "collapse" just set the width and height to zero. Maybe that has changed (or I misremembered) and now only the width is set to zero.

Comment 10

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

A simple testcase doesn't reproduce the problem for me :-(.

You might want to try flipping the changes to layout/xul/base/src/nsSprocketLayout.cpp in http://hg.mozilla.org/mozilla-central/rev/90e46b3e8e6f to IsEqualInterior instead of IsEqualEdges.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Sorry, this bug slipped off my radar for a while.  I spent some time a few weeks ago trying to construct a reduced test case, but hadn't got anything that reproduced the problem without including most of the Fennec chrome.

(In reply to comment #10)
> You might want to try flipping the changes to
> layout/xul/base/src/nsSprocketLayout.cpp in
> http://hg.mozilla.org/mozilla-central/rev/90e46b3e8e6f to IsEqualInterior
> instead of IsEqualEdges.

This does fix the regression (see attached patch).  Does this help in constructing a minimal test case, or should I keep trying to reduce one based on Fennec?

Comment 12

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

Comment on attachment 534047 [details] [diff] [review]
patch

Review of attachment 534047 [details] [diff] [review]:
-----------------------------------------------------------------

Let's just take this. This is essentially a very targeted backout.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 534047 [details] [diff] [review]
patch

Pushed to Try: http://tbpl.mozilla.org/?tree=Try&rev=50dcf61415d0

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: patch for checkin

No changes except to add a commit message. This passed Try on all platforms.

Comment 15

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/projects/cedar/rev/b0ad98f70c91

Comment 16

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/b0ad98f70c91

Comment 17

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

Thanks!

Comment 18

[Anna (Waverley)]

VERIFIED FIXED on:

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110524 Firefox/6.0a1 Fennec/6.0a1 

Device: HTC Desire Z (Android 2.2)