Make switching to and from about:customizing allow you to enter and exit customization mode

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The tab-strip is currently part of the group of things that are (visually speaking) available to be customized.

At the moment, the tab-strip can still be used - the background tabs are hidden behind a gradient, but they're still very much clickable.

UX would like to try making it so that clicking on a background tab exits you from customization mode, and clicking on the about:customizing tab (which would still be open) will re-enter you into that mode.
UX has requested we attempt to squeeze this into M2 if we have the chance.
Blocks: 855287
Created attachment 735446 [details] [diff] [review]
Patch v1

Hey Jared,

This seems to do the job. Care to take a peek?

-Mike
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attachment #735446 - Flags: review?(jaws)
Comment on attachment 735446 [details] [diff] [review]
Patch v1

Er, n/m - this breaks things horribly if entering customization mode via the panel.
Attachment #735446 - Attachment is obsolete: true
Attachment #735446 - Flags: review?(jaws)
Created attachment 735450 [details] [diff] [review]
Patch v1.1

Ok, this is better. There's a bug where if we have a about:customization tab open, and we open a new tab, and type in about:customization and press enter, the customization mode gets into a funny state (doubled items in the palette, inability to exit the mode).

I think I'd like to fix that in a follow-up, though.
Attachment #735450 - Flags: review?(jaws)
Comment on attachment 735450 [details] [diff] [review]
Patch v1.1

I think I've found a solution to that bug I mentioned in my last comment. Give me a second to try it...
Attachment #735450 - Flags: review?(jaws)
Created attachment 735499 [details] [diff] [review]
Patch v1.2

Alright, this should take care of this bug now (adds a rule such that we don't need to switch tabs, or create an about:customizing tab if the tab we're currently on is already at about:customizing).

As I mentioned to jaws in meatspace, I'm a tad worried that this patch could result in accidental tab-tearing when exiting customization mode via a tab switch... UX should give this a shot when it lands and let me know what they think.
Attachment #735450 - Attachment is obsolete: true
Attachment #735499 - Flags: review?(jaws)
Comment on attachment 735499 [details] [diff] [review]
Patch v1.2

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

::: browser/modules/CustomizeMode.jsm
@@ +196,5 @@
> +      let customizationTab = this.browser.selectedTab;
> +      if (this.browser.browsers.length == 1) {
> +        this.window.BrowserOpenTab();
> +      }
> +      this.browser.removeTab(customizationTab);

Most of the patch looks fine but as we talked about in person, we should probably not close the tab if the history of the tab is greater than zero. In that case, I think we should just navigate to the page previous to the customization.
Attachment #735499 - Flags: review?(jaws) → review-
Created attachment 735554 [details] [diff] [review]
Patch v1.3

Now using the history as an indicator for whether or not exiting customization mode should cause the browser to go back, or should close.
Attachment #735499 - Attachment is obsolete: true
Attachment #735554 - Flags: review?(jaws)
Comment on attachment 735554 [details] [diff] [review]
Patch v1.3

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

::: browser/modules/CustomizeMode.jsm
@@ +689,5 @@
> +   * Looks at the currently selected browser tab, and if the location
> +   * is set to kAboutURI and we're not customizing, enters customize mode.
> +   * If we're not at kAboutURI and we are customizing, exits customize mode.
> +   */
> +  _considerBrowser: function() {

Hm, I think we can find a better name for this function. Perhaps `toggleCustomizatoinModeIfNecessary()`?
Attachment #735554 - Flags: review?(jaws) → review+
Created attachment 735857 [details] [diff] [review]
Patch v1.4 (r+'d by jaws)

Yep, I dig the function name change. Thanks for the review.
Attachment #735554 - Attachment is obsolete: true
Attachment #735857 - Flags: review+
Landed on UX as https://hg.mozilla.org/projects/ux/rev/5805b8e97a98
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/933d587cfd04
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.