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

VERIFIED FIXED in mozilla2.0b12

Status

()

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: whimboo, Assigned: mossop)

Tracking

Trunk
mozilla2.0b12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker])

Attachments

(1 attachment, 3 obsolete attachments)

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.
blocking2.0: --- → ?
Assignee

Updated

9 years ago
blocking2.0: ? → betaN+
This is also happening in 

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

Updated

9 years ago
Assignee: nobody → dtownsend
Assignee

Updated

9 years ago
Depends on: 602256
Whiteboard: [soft blocker]
Assignee

Updated

9 years ago
Whiteboard: [soft blocker] → [softblocker]
Duplicate of this bug: 626207
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?
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

8 years ago
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.
Posted patch WIP (obsolete) — Splinter Review
On top of the patch in bug 602256 this seems to solve the problems for me.
Posted patch WIP (obsolete) — Splinter Review
Attachment #507274 - Attachment is obsolete: true
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/dtownsend@mozilla.com-e1ca8068d6e9/
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.
(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
(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.
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #507282 - Attachment is obsolete: true
Attachment #507728 - Flags: feedback?(bmcbride)
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?
Attachment #507728 - Flags: feedback?(bmcbride) → feedback+
(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 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
Attachment #507728 - Flags: review?(bmcbride)
Assignee

Updated

8 years ago
Whiteboard: [softblocker] → [softblocker][has patch][needs review Unfocused]
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.
Attachment #507728 - Attachment description: WIP → Patch v1
Attachment #507728 - Flags: review?(bmcbride) → review-
(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.
Assignee

Updated

8 years ago
Whiteboard: [softblocker][has patch][needs review Unfocused] → [softblocker][has patch]
Posted patch patch rev 2Splinter Review
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.
Attachment #507728 - Attachment is obsolete: true
Attachment #510326 - Flags: review?(bmcbride)
Assignee

Updated

8 years ago
Whiteboard: [softblocker][has patch] → [softblocker][has patch][needs review Unfocused]
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)
Attachment #510326 - Flags: review?(bmcbride) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/e2c895fafa27

Filed bug 632909 for the loading message follow-up.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [softblocker][has patch][needs review Unfocused] → [softblocker]
Target Milestone: --- → mozilla2.0b12
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
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.