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

VERIFIED FIXED in mozilla2.0b5

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mossop)

Tracking

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

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(Whiteboard: [rewrite])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Summary: Add-ons manager doesn't integrate with session history → Add-ons manager doesn't integrate with session history (back button misbehaves)

Comment 1

7 years ago
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

7 years ago
correction: it sounds like Jesse wants the back button to switch back to the addons window
(Reporter)

Comment 3

7 years ago
I'm talking about the new add-ons manager that just landed on trunk.
(Reporter)

Comment 4

7 years ago
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.
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.
Keywords: uiwanted
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [rewrite]
Blocks: 550048
(Reporter)

Comment 6

7 years ago
See also bug 560449.
Blocks: 560449
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.
Keywords: uiwanted
Summary: Add-ons manager doesn't integrate with session history (back button misbehaves) → Add-ons manager doesn't integrate with session history (back/forward button misbehaves)
(Assignee)

Updated

7 years ago
Blocks: 565359
blocking2.0: --- → ?
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.
(Assignee)

Updated

7 years ago
Depends on: 571970
(Assignee)

Comment 9

7 years ago
Once the browser chrome goes away in bug 571970 this becomes a non-issue I think
blocking2.0: ? → -
(Reporter)

Comment 10

7 years ago
What if I press Cmd+[?  What about the bugs that are currently marked as depending on this bug?
(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.
blocking2.0: - → ?
(Assignee)

Comment 12

7 years ago
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.
No longer blocks: 560449
blocking2.0: ? → -
(Assignee)

Comment 13

7 years ago
After further discussion with UX I think we're going to have to fix this after all.
blocking2.0: - → final+
(Assignee)

Comment 14

7 years ago
Created attachment 455829 [details] [diff] [review]
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #455829 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 557949

Comment 16

7 years ago
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 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.
Attachment #455829 - Flags: review?(bmcbride) → review-
(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.
(Assignee)

Comment 19

7 years ago
This is blocking adding the forward/back buttons to the UI which probably want tooltips so needs to make the string freeze.
blocking2.0: final+ → beta5+
(Assignee)

Updated

7 years ago
No longer depends on: 571970
(Assignee)

Updated

7 years ago
Duplicate of this bug: 590192
(Assignee)

Comment 21

7 years ago
Created attachment 469163 [details] [diff] [review]
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.
Attachment #455829 - Attachment is obsolete: true
Attachment #469163 - Flags: review?(bmcbride)
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.
Attachment #469163 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 23

7 years ago
(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.
(Assignee)

Comment 24

7 years ago
Created attachment 469682 [details] [diff] [review]
patch rev 3

Updated and with styling done on Linux and Windows.
Attachment #469163 - Attachment is obsolete: true
Attachment #469682 - Flags: review?(bmcbride)
(Assignee)

Comment 25

7 years ago
Created attachment 469974 [details] [diff] [review]
patch rev 4

This patch applies cleanly by itself, Shawn how do you feel about reviewing this so we can get it landed today?
Attachment #469682 - Attachment is obsolete: true
Attachment #469974 - Flags: review?(sdwilsh)
Attachment #469682 - Flags: review?(bmcbride)
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
Attachment #469974 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 27

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/68fa23925a1f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5

Updated

7 years ago
Depends on: 591801
Depends on: 591895
Depends on: 591905
We have a couple of regression bugs since its landing. Shall we better back it out for beta 5?
(Assignee)

Updated

7 years ago
No longer depends on: 591895
(Assignee)

Comment 29

7 years ago
(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.
(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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
(Assignee)

Comment 31

7 years ago
The tests got disabled because they were timing out, once I fix that in bug 591566 this will have good automated testing.
Flags: in-litmus? → in-litmus-
(Assignee)

Comment 32

7 years ago
Well tested by browser_bug562797.js
Flags: in-testsuite? → in-testsuite+
Created attachment 531508 [details] [diff] [review]
backspace navigation tests

Adds tests for backspace navigation (Bug 565359)
Attachment #531508 - Flags: review?(dtownsend)
(Assignee)

Comment 34

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

Review of attachment 531508 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531508 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 35

6 years ago
Landed the new test at http://hg.mozilla.org/mozilla-central/rev/5be875928599
You need to log in before you can comment on or make changes to this bug.