562797 - Add-ons manager doesn't integrate with session history (back/forward button misbehaves)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jesse Ruderman]

1. Tools > Add-ons > Extensions
2. Double-click on an extension (or right-click and select "show more information")
3. Click the browser's back button

Result: ???

Expected: take me back to the list of extensions

Comment 1

[Gubment Cheez]

unable to repro on windows 3.6.3 . double-clicking extension does nothing, "Visit home page" opens new page in window with proper browsing history <can go back to previous page, if one was open>, and "About <extension>" opens modal window. it sounds like Jesse wants the window to switch back to the addons window, which would be unintuitive to me.

Comment 2

[Gubment Cheez]

correction: it sounds like Jesse wants the back button to switch back to the addons window

Comment 3

[Jesse Ruderman]

I'm talking about the new add-ons manager that just landed on trunk.

Comment 4

[Jesse Ruderman]

It's probably ideal if about:addons uses hash-URLs.  That way, the following can work:

1. Tools > Add-ons > Extensions
2. Double-click an extension
3. Close the tab.
4. Cmd+Shift+T (reopen last closed tab)

Result: Taken to the list of extensions.

Expected: Taken to the extension I was viewing.

Comment 5

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

Jesse is talking about the new addons manager which has been landed a couple of hours before on trunk.

See also bug 557949. We would need a solution if we want to use session history or not. I think it would be a great idea and will improve the work with the addons manager.

Comment 6

[Jesse Ruderman]

See also bug 560449.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

The expected behavior should be as Jesse stated.

A change to the right panel of the add-ons manager should be equivalent to opening a new page; back and forward should move the right panel accordingly.

Also, add-ons should expand with a single rather than double click - I'll file a bug on that.

Comment 8

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100610 SeaMonkey/2.1a2pre

Reproducible: Always

Steps to Reproduce:
0. Open addons manager ("Tools => Add-on Manager" or browse to about:addons)
1. Make sure you have enough extensions to be able to scroll down the summary view.
2. Scroll down the summary view
3. Double-click an extension.
4. Click "Back to Extensions" in extension manager titlebar (in content)

Expected results:
scrolled down same as at step 2

Actual results:
Addons manager reopens scrolled all the way to the top

Additional info:
I would have thought this was a different bug (comment #0 and comment #4 make no mention of scroll position) but whimboo dissuaded me.

Comment 9

[Dave Townsend [:mossop]]

Once the browser chrome goes away in bug 571970 this becomes a non-issue I think

Comment 10

[Jesse Ruderman]

What if I press Cmd+[?  What about the bugs that are currently marked as depending on this bug?

Comment 11

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

(In reply to comment #10)
> What if I press Cmd+[?  What about the bugs that are currently marked as
> depending on this bug?

I have to agree. Even with the browser chrome gone we will still have the keyboard shortcuts. And I don't think that they will be removed.

Comment 12

[Dave Townsend [:mossop]]

And I don't believe that is enough to block the release on. Also Bug 560449 isn't blocked by this as far as I can tell.

Comment 13

[Dave Townsend [:mossop]]

After further discussion with UX I think we're going to have to fix this after all.

Comment 14

[Dave Townsend [:mossop]]

Attachment: patch rev 1

So HTML5's session history stuff makes this reasonably straightforward. The tricky parts are hooking up the header and category correctly particularly for the search cases.

To save you reading the spec, when a page is loaded you get load, pageshow and popstate events in that order. So this makes us initialise at load, pageshow is left for the app to tell us to load a specific view and if no pane has been loaded when we reach popstate we load the default view.

Whenever we load a new view we stash the new and previous views in the history state. When returning popstate gives us that back and we can use it to load the view and set the category and header correctly.

Comment 16

[Josh Triplett]

Rather than using HTML5 session history, why not simply change the URI (either path or anchor)?  That has the added advantage of making the location bar reflect the current location, making it possible to copy and paste.  (For example, an add-on developer could say "go to about:addons#my-addon-id".)

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 455829 [details] [diff] [review]
patch rev 1

Ugh, sorry this took so ridiculously long. 

I've been thinking about wanting some additional things here, but really they're out of the scope of this bug, and probably don't deserve to be blockers either. eg:
* Providing a way for individual views to store their state
* Have the list view store its scroll position and selected item
* Having the search view store the remote addons (rather than using a global variable to store one-level of history)


So for the scope of this bug, it looks good. Couple of things I'd like to see refactored though:
* Move gInitialViewSelected into gViewController, so its not a global (there's been a creep of globals lately).
* Move the popstate event handler into gViewController - and move the addEventListener call into gViewController.initialize()

ie, move anything relevant into gViewController - since this is all about the views.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #16)
> Rather than using HTML5 session history, why not simply change the URI (either
> path or anchor)?  That has the added advantage of making the location bar
> reflect the current location, making it possible to copy and paste.  (For
> example, an add-on developer could say "go to about:addons#my-addon-id".)

There's quite a lot of state to be kept here - too much for a user-facing URL, IMO. eg: What type of view, what type of addon the view is to display, parent view, etc. And other things that'd be nice to have in the future, eg: scroll positions, what item is selected, a cache of available addons from AMO, etc.

But this doesn't preclude the possibility of supporting about:addons#my-addon-id as a much simpler shortcut that just always assumes you want the detail view of a specific addon.

Comment 19

[Dave Townsend [:mossop]]

This is blocking adding the forward/back buttons to the UI which probably want tooltips so needs to make the string freeze.

Comment 21

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated to trunk and replaces the header text with forward/back buttons. The buttons aren't styled on Linux and Windows yet, that will be easier for me to do in the office tomorrow but I figure that needn't hold up review of the rest of the code.

Your main points have been addressed. I've added some hacky code to make it so you can't go forward to the detail view of an add-on you've just clicked to uninstall, it isn't ideal but we likely need a platform fix to be able to do that properly.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Had some trouble applying this patch - was it on top of any patch other than bug 562902? Or just on top of an updated patch for bug 562902?

Also, something broke the entire addons manager UI when I first ran a build with this patch. Haven't been able to reproduce it since. And stupid me forgot to save the error logs. But from what I remember, it looked like one of the  functions called on startup broke, when then caused everything else to break (or at least, not start up, and therefore not work correctly).

>+  }
>+  else {

General style nit: No newline.

>+function loadView(aViewId) {
>+  if (gViewController.initialViewSelected) {
>+    gViewController.loadView(aViewId);
>+  }
>+  else {
>+    // The caller opened the window and immediately loaded the view so it
>+    // should be the initial history entry
>+    gViewController.loadInitialView(aViewId, true, null);
>+  }

Nit: It would be easier to read if these were swapped (special case before the generic case). ie, if the call to loadInitialView were in the first block.

>+  statePopped: function(e) {
>+    // If this is a navigation to a previous state then load that state
>+    if (e.state) {
>+      this.loadInitialView(e.state.view, false, e.state.previousView);
>+      return;
>+    }

If this is a navigation back/forward, then its not the initial view, right? So why does it call loadInitialView?

>+  // Moves back in the document history and removes the current history entry
>+  popState: function(aCallback) {
>+    this.viewChangeCallback = function() {
>+      // TODO To ensure we can't go forward again we put an additional entry for
>+      // the current page into the history. Ideally we would just strip the
>+      // history but there doesn't seem to be a way to do that. Bug 590661
>+      window.history.pushState({
>+        view: gViewController.currentViewId,
>+        previousView: gViewController.currentViewId
>+      }, document.title);
>+      this.updateCommands();
>+
>+      if (aCallback)
>+        aCallback();
>+    };
>+    window.history.back();
>+  },

This makes me sad.

Especially since you really need to go back twice before pushing a state. With this as-is, when you uninstall, then click the back button, it reloads the view that just loaded. Still, this is far better than nothing.

>+    <button id="back-btn" class="nav-button" command="cmd_back"
>+            accesskey="&cmd.back.accesskey;" tooltiptext="&cmd.back.tooltip;"/>
>+    <button id="forward-btn" class="nav-button" command="cmd_forward"
>+            accesskey="&cmd.forward.accesskey;" tooltiptext="&cmd.forward.tooltip;"/>

Do these really need accesskeys? The usual back/forward shortcuts work fine (alt+left arrow, alt+right arrow), so additional accesskeys are redundant. Plus they're not actually exposed anywhere for them to be discoverable.

>+/**
>+ * Tests that history navigation works for the add-ons manager. Only used if
>+ * in-content UI is supported for this application.
>+ */

Why?

>+function is_in_list(aManager, view, title, canGoBack, canGoForward) {

title doesn't seem to be actually used in this function.

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> Comment on attachment 469163 [details] [diff] [review]
> patch rev 2
> 
> Had some trouble applying this patch - was it on top of any patch other than
> bug 562902? Or just on top of an updated patch for bug 562902?

Might have been on top of an update version of bug 562902, I'll have that up shortly anyway.

> Also, something broke the entire addons manager UI when I first ran a build
> with this patch. Haven't been able to reproduce it since. And stupid me forgot
> to save the error logs. But from what I remember, it looked like one of the 
> functions called on startup broke, when then caused everything else to break
> (or at least, not start up, and therefore not work correctly).

I've not seen this myself :s

> >+  }
> >+  else {
> 
> General style nit: No newline.

One day I will learn.

> >+function loadView(aViewId) {
> >+  if (gViewController.initialViewSelected) {
> >+    gViewController.loadView(aViewId);
> >+  }
> >+  else {
> >+    // The caller opened the window and immediately loaded the view so it
> >+    // should be the initial history entry
> >+    gViewController.loadInitialView(aViewId, true, null);
> >+  }
> 
> Nit: It would be easier to read if these were swapped (special case before the
> generic case). ie, if the call to loadInitialView were in the first block.

Done.

> >+  statePopped: function(e) {
> >+    // If this is a navigation to a previous state then load that state
> >+    if (e.state) {
> >+      this.loadInitialView(e.state.view, false, e.state.previousView);
> >+      return;
> >+    }
> 
> If this is a navigation back/forward, then its not the initial view, right? So
> why does it call loadInitialView?

Yeah it doesn't, corrected and that simplifies things slightly.

> >+  // Moves back in the document history and removes the current history entry
> >+  popState: function(aCallback) {
> >+    this.viewChangeCallback = function() {
> >+      // TODO To ensure we can't go forward again we put an additional entry for
> >+      // the current page into the history. Ideally we would just strip the
> >+      // history but there doesn't seem to be a way to do that. Bug 590661
> >+      window.history.pushState({
> >+        view: gViewController.currentViewId,
> >+        previousView: gViewController.currentViewId
> >+      }, document.title);
> >+      this.updateCommands();
> >+
> >+      if (aCallback)
> >+        aCallback();
> >+    };
> >+    window.history.back();
> >+  },
> 
> This makes me sad.
> 
> Especially since you really need to go back twice before pushing a state. With
> this as-is, when you uninstall, then click the back button, it reloads the view
> that just loaded. Still, this is far better than nothing.

I briefly considered going back twice and then pushing but quickly realised that might mean going back out of the add-ons manager and to a webpage which will probably do terribly things. As far as I can tell at the moment hte options are either to clear all history or to have this double-back case. Going to talk to more knowledgeable people to see if I've missed something though.

> >+    <button id="back-btn" class="nav-button" command="cmd_back"
> >+            accesskey="&cmd.back.accesskey;" tooltiptext="&cmd.back.tooltip;"/>
> >+    <button id="forward-btn" class="nav-button" command="cmd_forward"
> >+            accesskey="&cmd.forward.accesskey;" tooltiptext="&cmd.forward.tooltip;"/>
> 
> Do these really need accesskeys? The usual back/forward shortcuts work fine
> (alt+left arrow, alt+right arrow), so additional accesskeys are redundant. Plus
> they're not actually exposed anywhere for them to be discoverable.
> 
> >+/**
> >+ * Tests that history navigation works for the add-ons manager. Only used if
> >+ * in-content UI is supported for this application.
> >+ */
> 
> Why?

I guess it only made sense that way, but I suppose there is no reason not to test the other case too.

> >+function is_in_list(aManager, view, title, canGoBack, canGoForward) {
> 
> title doesn't seem to be actually used in this function.

Right, the header title is gone now.

Comment 24

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Updated and with styling done on Linux and Windows.

Comment 25

[Dave Townsend [:mossop]]

Attachment: patch rev 4

This patch applies cleanly by itself, Shawn how do you feel about reviewing this so we can get it landed today?

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 469974 [details] [diff] [review]
patch rev 4

on file: toolkit/mozapps/extensions/content/extensions.js line 134
>     // The caller opened the window and immediately loaded the view so it
>     // should be the initial history entry

global-nit: period at the end please


on file: toolkit/mozapps/extensions/test/browser/Makefile.in line 55
>   browser_bug562797.js \

ew, you guys don't use descriptive names for your tests? :(


r=sdwilsh

Comment 27

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/68fa23925a1f

Comment 28

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

We have a couple of regression bugs since its landing. Shall we better back it out for beta 5?

Comment 29

[Dave Townsend [:mossop]]

(In reply to comment #28)
> We have a couple of regression bugs since its landing. Shall we better back it
> out for beta 5?

None of the regressions would block beta 5 so no this won't be backed out.

Comment 30

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

(In reply to comment #29)
> None of the regressions would block beta 5 so no this won't be backed out.

Right, those are independent or handled separately now. The initial landing of this feature looks good. Everything I was able to detect has been filed as new bugs.

So lets call this verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre

Dave, which tests do we need here? How well is it covered on the automation side?

Comment 31

[Dave Townsend [:mossop]]

The tests got disabled because they were timing out, once I fix that in bug 591566 this will have good automated testing.

Comment 32

[Dave Townsend [:mossop]]

Well tested by browser_bug562797.js

Comment 33

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: backspace navigation tests

Adds tests for backspace navigation (Bug 565359)

Comment 34

[Dave Townsend [:mossop]]

Comment on attachment 531508 [details] [diff] [review]
backspace navigation tests

Review of attachment 531508 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 35

[Dave Townsend [:mossop]]

Landed the new test at http://hg.mozilla.org/mozilla-central/rev/5be875928599