591905 - Back/Forward buttons do not always update selected category / pane when "Get Add-ons" has been chosen before

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre

This was kinda hard to nail down but now I can reproduce it all the time. The reason why the session history for the Add-ons Manager is sometimes confused is probable the loading of external pages inside the frame of "Get Add-ons". Once this pane is in the session history list, sometimes the category button and even the whole panel doesn't get updated.

Steps:
1. Open the Add-ons Manager with the extension pane selected
2. Quickly click: "Get Add-ons", "Extensions", "Themes", "Plugins"
3. Use the back and forward buttons to walk through the list of session entries

With step 3 you will notice that some of the entries in the session history do not get opened. The panel / category selection of the last element is still shown. Going forward will show those instead.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

This is also happening in 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8) Gecko/20100101 Firefox/4.0b8

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Now with the details view of the Get Addons pane landed this bug is even easier to reproduce. Given the broken navigation can we consider it as a hardblocker?

Comment 4

[Dave Townsend [:mossop]]

We did have to put quite a lot of thought into whether this is soft or hard blocking however ultimately we decided on soft since although the back/forward buttons end up a little messed up navigation is still possible through the rest of the UI.

I'm still going to try to get this fixed regardless but at the moment I am mostly stuck without figuring out what is going on with bug 602256

Comment 5

[Joe Theuerkauf]

Carrying my comment over from bug 606459
Dave, i'd humbly suggest that navigation is technically still possible after the history gets messed up, but only by closing the tab & going back to the Firefox menu, or going to the location bar and hitting Enter to make the current location (about:addons) the newest history entry (& make sure you were on the "Get Addons" panel before you do so).

Aside from that, once history/navigation is messed up & you're on an Addon's detail page in the "Get Addons" panel, there's no way to get off the details page (even a Back link as part of the detail page template would suffice).

Thanks for your patience on my misdirection. Looking forward to beta 11 (kidding, hopefully). Good luck.

Comment 6

[Dave Townsend [:mossop]]

Attachment: WIP

On top of the patch in bug 602256 this seems to solve the problems for me.

Comment 7

[Dave Townsend [:mossop]]

Attachment: WIP

Comment 8

[Dave Townsend [:mossop]]

Made some test builds for this on the try server, might be helpful for others to experiment with them and report if they see issues with this stuff

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e1ca8068d6e9/

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This build works good so far on OS X. Somehow I was able to catch two situations I'm not able to reproduce constantly. Both are probably not related to your patch.

1. The Back/Forward buttons disappear and they don't come back. You will have to reopen the Add-ons Manager

2. I end-up with a blank Get Addons pane while quickly navigating forward and backward through a couple of formerly selected panes.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> This build works good so far on OS X. Somehow I was able to catch two
> situations I'm not able to reproduce constantly. Both are probably not related
> to your patch.
> 
> 1. The Back/Forward buttons disappear and they don't come back. You will have
> to reopen the Add-ons Manager

Just figured this out and filed bug 629418

> 2. I end-up with a blank Get Addons pane while quickly navigating forward and
> backward through a couple of formerly selected panes.

That's probably related to this patch though I thought it had fixed it. Getting definite STR for this would be helpful

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #10)
> > 2. I end-up with a blank Get Addons pane while quickly navigating forward and
> > backward through a couple of formerly selected panes.
> 
> That's probably related to this patch though I thought it had fixed it. Getting
> definite STR for this would be helpful

Sorry, but I wasn't able to reproduce this behavior. I had some network issues this afternoon and in-between I have also seen the placeholder page for offline mode. Not sure if this could be related.

Comment 12

[Dave Townsend [:mossop]]

Attachment: Patch v1

Basically there but since bug 602256 isn't done yet just wanted to get some feedback here.

The bulk of the patch is making history work properly when we're running in a window and so have fake history. In order to do that I've made it so the show method of views gets the history state, this allows us to store the url in there for the discover view and reload it when asked. An alternative was to just append the url to the view (addons://discover/https://....) but that seemed needlessly ugly and besides I can think of lots of things we may want to stash in the history to restore (selected item in the list f.e.) so this would come in useful in the future.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 507728 [details] [diff] [review]
Patch v1

Sounds good.

Would prefer if you kept document.title in the pushState/replaceState calls (in case it ever does change, eg by an addon) - unless it magically fetches that for you?

Why is LOAD_FLAGS_REPLACE_HISTORY being bitshifted?

Comment 14

[Dave Townsend [:mossop]]

(In reply to comment #13)
> Comment on attachment 507728 [details] [diff] [review]
> WIP
> 
> Sounds good.
> 
> Would prefer if you kept document.title in the pushState/replaceState calls (in
> case it ever does change, eg by an addon) - unless it magically fetches that
> for you?

THe gHistory implementations never accepted a title parameter so they are just useless.

> Why is LOAD_FLAGS_REPLACE_HISTORY being bitshifted?

The docshell load type is a combination of the flags passed to the load and a load type, see http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShellLoadTypes.h#56

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 507728 [details] [diff] [review]
Patch v1

The other bug has passed review with no changes so this needs a full review now. I responded to the feedback comments and don't think they mean any changes

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 507728 [details] [diff] [review]
Patch v1

The code in gDiscoverView is getting a bit twisty-turny. eg, the special-casing for home page vs non home page loads - one will show the loading message, the other won't. Would be a lot simpler/nicer if that difference were removed, and _loadDefaultPage() just passed in the homepage URL to _loadURL().

+    if (gHistory == FakeHistory) {

This check feels kinda awkward, and it isn't obvious why the different behavior is needed. Not sure if checking a property on gHistory makes more sense or not, but it at least needs a comment.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> +    if (gHistory == FakeHistory) {
> 
> This check feels kinda awkward, and it isn't obvious why the different behavior
> is needed. Not sure if checking a property on gHistory makes more sense or not,
> but it at least needs a comment.

I've added a comment here, not sure there is a better way to test this except by maybe re-doing the existence of session history check that we have elsewhere to decide what kind of history to use, and that seems like a waste of time.

Comment 18

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated from the comments on IRC. Now shows the loading page whenever loading which feels kind of jerky to me but it does make the code cleaner.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 510326 [details] [diff] [review]
patch rev 2

Looks good. 

Could you file a followup bug to fade in the loading message, like how the details view does? (Out of scope for this bug, IMO)

Comment 20

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/e2c895fafa27

Filed bug 632909 for the loading message follow-up.

Comment 21

[Dave Townsend [:mossop]]

Followed up with a fix to stop the browser re-loading when going back to the url it already shows. This was making tests fail for the windowed add-ons manager.

http://hg.mozilla.org/mozilla-central/rev/0f10a0099dfc

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre