Closed Bug 564285 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Windows 7
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: vingtetun)

References

Details

(Whiteboard: rc1.1)

Attachments

(1 file, 1 obsolete file)

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
Attached patch Patch (obsolete) — Splinter Review
(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
Attached patch PatchSplinter Review
(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
Closed: 9 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
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.