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

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
5 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: vingtetun)

Tracking

Trunk
x86
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus +

Details

(Whiteboard: rc1.1)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
This is a locked toolbar regression. Likely from bug 537717 or maybe from bug 564075.
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)
Assignee: nobody → 21
(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 :(
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.
tracking-fennec: --- → ?
Whiteboard: rc1.1
Created attachment 446503 [details] [diff] [review]
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)
Attachment #446503 - Flags: review?(mark.finkle)
Attachment #446503 - Flags: review?(mark.finkle) → review-
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
Created attachment 446521 [details] [diff] [review]
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)
Attachment #446503 - Attachment is obsolete: true
Attachment #446521 - Flags: review?(mark.finkle)
Attachment #446521 - Flags: review?(mark.finkle) → review+
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
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
Blocks: 526564
Assignee: 21 → aaron.train

Updated

8 years ago
Assignee: aaron.train → 21
Flags: in-litmus? → in-litmus?(aaron.train)
https://litmus.mozilla.org/show_test.cgi?id=12905
Flags: in-litmus?(aaron.train) → in-litmus+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.