Closed Bug 470989 Opened 16 years ago Closed 15 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0b2

People

(Reporter: abillings, Assigned: vingtetun)

References

Details

Attachments

(3 files, 6 obsolete files)

Attached image 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.
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.
Summary: Opening and closing bookmark dialog can cause right panel to scroll up and down → Using autocomplete to select page causes right panel to scroll up and down
Attached image example of bug
It would be nice if someone looked at this after nine months...
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
Attached patch Dirty patch who do the work (obsolete) — Splinter Review
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 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.
Attachment #383718 - Flags: review-
Attached patch Patch wich move panHandler (obsolete) — Splinter Review
(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?)
Did the fix to make panBy round the parameters yesterday help with this?
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.
Attached patch Update patch (obsolete) — Splinter Review
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
Attachment #383958 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
Add changes suggested by bcombee
Attachment #384010 - Attachment is obsolete: true
Attachment #384019 - Flags: review?(gavin.sharp) → review-
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+
Attached patch Updated patch (obsolete) — Splinter Review
Revert back panHandler to browser.js
Attachment #384019 - Attachment is obsolete: true
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 :)
Thanks gavin for figure it out. It makes much more sense that my workaround!
Attachment #384098 - Attachment is obsolete: true
Assignee: nobody → 21
OS: Linux → All
Hardware: ARM → All
Target Milestone: --- → B2
https://hg.mozilla.org/mobile-browser/rev/617eb3b860ff

Merci encore Vivien! :)
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Verified fixed in my build today.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: