564285 - Remember password infobar is overlapped by the url bar, while loading

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

To reproduce, use bugzilla as testing site. Make sure you've logged out at bugzilla and make sure Fennec's password manager hasn't remembered it (otherwise remove the entry).

- Open https://bugzilla.mozilla.org/enter_bug.cgi?product=Fennec
- You get the login page of bugzilla
- Type in your user name and password, and submit it.

Expected result:
- The url bar shows up while the new page is loading and the remember password infobar shows up completely visible, while the new page is loading.

Actual result:
- The url bar covers the remember password infobar, while the new page is loading.

Comment 1

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

This is a locked toolbar regression. Likely from bug 537717 or maybe from bug 564075.

Comment 2

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

Vivien, see if you can find the problem and make a patch. We should try to land something to fix this by RC code freeze (may 13th).

If there is no elegant solution, we might need to unlock the toolbar when the notification is shown. We have an event handler for that:

http://hg.mozilla.org/mobile-browser/file/0596318de8f9/chrome/content/browser.js#l456

(sorry, MXR was down)

Comment 3

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

(In reply to comment #2)
> Vivien, see if you can find the problem and make a patch. We should try to land
> something to fix this by RC code freeze (may 13th).
> 
> If there is no elegant solution, we might need to unlock the toolbar when the
> notification is shown. We have an event handler for that:
> 
> http://hg.mozilla.org/mobile-browser/file/0596318de8f9/chrome/content/browser.js#l456
> 
> (sorry, MXR was down)

I've look at it today but there is no elegant solution I can find quickly.
I think, I'll rely on unlocking the toolbar when there is a notification but that's rude :(

Comment 4

[Aakash Desai [:aakashd]]

I think this is the same as https://bugzilla.mozilla.org/show_bug.cgi?id=525115 . If so, I can just dupe the other to this one since there's more content here.

Comment 5

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

Attachment: Patch

(In reply to comment #4)
> I think this is the same as https://bugzilla.mozilla.org/show_bug.cgi?id=525115
> . If so, I can just dupe the other to this one since there's more content here.

This is a different bug.

(In reply to comment #2)
> Vivien, see if you can find the problem and make a patch. We should try to land
> something to fix this by RC code freeze (may 13th).
> 
> If there is no elegant solution, we might need to unlock the toolbar when the
> notification is shown. We have an event handler for that:
> 

The patch prevent locking the toolbar if there a notification when loading a new page. (the notificationHandler is fired before networkStart)

Comment 6

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

Comment on attachment 446503 [details] [diff] [review]
Patch

>diff -r 7a40d5b5552a chrome/content/browser-ui.js

>-        this.unlockToolbar();
>+        if (this._shouldUnlockToolbar) {
>+          this._shouldUnlockToolbar = false;
>+          this.unlockToolbar();
>+        }

Can we just use | this.isToolbarLocked() |instead of _shouldUnlockToolbar ?

>         break;
> 
>       case TOOLBARSTATE_LOADING:
>         if (icons.getAttribute("mode") != "edit")
>           this._updateToolbar();
> 
>         browser.mIconURL = "";
>         this._updateIcon();
>-        this.lockToolbar();
>+
>+        if (!Browser.hasNotificationsForTab(Browser.selectedTab)) {
>+          this._shouldUnlockToolbar = true;
>+          this.lockToolbar();
>+        }

Sounds like hasNotificationsForTab should be on the notificationbox XBL, but I guess we don't override that. OK, leave on Browser.

>+  hasNotifications: function(aBrowser) {
>+  },
>+

Remove this

Comment 7

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

Attachment: Patch

(In reply to comment #6)
> (From update of attachment 446503 [details] [diff] [review])
> >diff -r 7a40d5b5552a chrome/content/browser-ui.js
> 
> >-        this.unlockToolbar();
> >+        if (this._shouldUnlockToolbar) {
> >+          this._shouldUnlockToolbar = false;
> >+          this.unlockToolbar();
> >+        }
> 
> Can we just use | this.isToolbarLocked() |instead of _shouldUnlockToolbar ?

We can't because there is no other reliable way to have a balanced lockToolbar/unlockToolbar.
I've tried to work around that but every others solutions have a way to have unwanted unlockToolbar call.

I'm thinking of changing a bit the architecture of the scrollbox for fennec 2.0 which will helps regarding this kind of bug with the notification bar (and also the find bar)

Comment 8

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

Comment on attachment 446521 [details] [diff] [review]
Patch

I don't like the _shouldUnlockToolbar flag at all. But if this is the quick and easy way to fix it, then so be it.

I do want to change the name of _shouldUnlockToolbar to something less general and more specific.

I'll do it on checkin

Comment 9

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

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/c3bdd9de0de5

pushed to m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/1a1d3ca18b51

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100521 Namoroka/3.6.5pre Fennec/1.1b2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100521 Namoroka/3.7a5pre Fennec/2.0a1pre

Comment 11

[Aaron Train [:aaronmt]]

https://litmus.mozilla.org/show_test.cgi?id=12905