Status

()

The Mozilla Toolkit is a set of APIs, built on top of Gecko, which provide advanced services to XUL applications. These services include Profile Management, Chrome Registration, Browsing History, Extension and Theme Management, Application Update Service, and Safe Mode. (More info)

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.

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.

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.

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.

(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.

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.

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.

(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.

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 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.

(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 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

(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?