470989 - Using autocomplete to select page causes right panel to scroll up and down

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Al Billings [:abillings - ex-MoCo]]

Attachment: screenshot of bug

I've noticed on certain pages that if the dialog to add or edit a bookmark is opened and dismissed, a gap will open up at the top of the right panel in the UI. See attached screenshot. 

This was found with the Mozilla/5.0 (X11; U; Linux a mv6l; en-US; rv:1.9.2a1pre)
Gecko/20081223 Fennec/1.0a2.

Steps to Reproduce
1. Start Fennec
2. Go to about:
3. Open the right panel and click on the star button for the bookmark dialog
4. Select the "Done" button

Result: a gap will render at the top of the right panel. If the page is scrolled up and down, it can be seen as well.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

Repro steps have changed on this one.

1. Start Fennec
2. Type in the addressbar and select page from history/bookmarks in dropdown options.
3. Pull fennec to the left to show right panel
4. Scroll page content up and down.

Result: Background behind UI will show as top portion of UI and right panel are disconnected. See attached graphic.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Attachment: example of bug

It would be nice if someone looked at this after nine months...

Comment 3

[Ben Combee (:bcombee)]

Very similar problem:

1) Start Fennec
2) Click in awesome bar, go to news.google.com
3) Scroll down a bit
4) Go back a page to firstrun screen
5) Scroll page to right to see tab bar
6) Title bar isn't locked on screen, so you can scroll up to see checkboard

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Dirty patch who do the work

This is a little dirty patch that correct the problem.

The bug came from window.gSidebarVisible which is not turn to false when we load a new page or switch tab. 
This situtation lead to not call showToolbar(URLBAR_FORCE) even is the sidebar are visible after a first time we have show them.

Comment 5

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

Comment on attachment 383718 [details] [diff] [review]
Dirty patch who do the work

Can you just move "panHandler" to BrowserUI? Hook it up in the BrowserUI.init method.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch wich move panHandler

(In reply to comment #5)
> (From update of attachment 383718 [details] [diff] [review])
> Can you just move "panHandler" to BrowserUI? Hook it up in the BrowserUI.init
> method.

Here a quick patch, but I think a bit there is much more work to do here.
There is some strange behavior with theses sidebar mainly do to endKinetic:
 * It fires AFTER panHandler and can hide the sidebar and break the flag (I'm searching a clean way to completely remove this flag)
 * The panBy value are sometimes not perfect and isWidgetVisible continue to say the widget are visible even if we have try to hide them

Should we open a new bug for what I described? (or handles all theses issues here?)

Comment 7

[Ben Combee (:bcombee)]

Did the fix to make panBy round the parameters yesterday help with this?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

It seems to help a little bit but it seems to not completely fullfill my needs - In isWidgetVisible I can see that the width of the sidebar is 0.19.... for example.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Update patch

Add 2 math.round in _dragBy in WidgetStack
Add a isWidgetFrozen method in WidgetStack
Remove isSidebarVisibleFlag
Add a call to unfreeze the toolbar if needed in endKinetic

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Add changes suggested by bcombee

Comment 11

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

Comment on attachment 384019 [details] [diff] [review]
Updated patch

Vivien - This looks good, except I made a mistake about moving "panHandler" to BrowserUI (especially now that the code uses only BrowserUI.showToolbar).

Also, Stuart has a patch in the works, for something different, and he would like to keep "panHandler" in Browser for that patch.

Update a new patch and you'll get the r+

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Revert back panHandler to browser.js

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Move Math.round

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Turns out the only reason the pannableBounds end up having non-integer coordinates is because we fail to round the viewportOverflow rect (the viewportBounds are already run through Math.ceil before setViewportBounds is called in browser.js). I'd like to avoid removing the rounding in panBy and instead just add make this change in WidgetStack.js:

     this._viewportOverflow = new wsBorder(
-      /*top*/ Math.min(ofRect.top, 0),
-      /*left*/ Math.min(ofRect.left, 0),
-      /*bottom*/ Math.max(ofRect.bottom - vp.rect.height, 0),
-      /*right*/ Math.max(ofRect.right - vp.rect.width, 0)
+      /*top*/ Math.round(Math.min(ofRect.top, 0)),
+      /*left*/ Math.round(Math.min(ofRect.left, 0)),
+      /*bottom*/ Math.round(Math.max(ofRect.bottom - vp.rect.height, 0)),
+      /*right*/ Math.round(Math.max(ofRect.right - vp.rect.width, 0))

Apart from that the patch looks good to me. Hope you're not getting tired of dealing with all these change requests :)

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Incorporate the gavin's comment

Thanks gavin for figure it out. It makes much more sense that my workaround!

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

/that/than/g

Comment 17

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mobile-browser/rev/617eb3b860ff

Merci encore Vivien! :)

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in my build today.