754304 - make in-content preferences linkable via URL

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement)

Description

[dindog]

Since we landed In-Content Preference now, that would be nice to have URI access to specific option as anchor to regular HTML.

That would be very helpful for support. For example, 

Open Option then focus at HarewareAcceleration setting:
chrome://option/advanced/HarewareAcceleration

or a link like 
chrome://option/advanced/SmoothScroll#disabled
to disable smooth scroll.

Comment 1

[Willy_ Foo_Foo]

wouldn't be it better if instead of using chrome://option/advanced/HarewareAcceleration

its used about:HarewareAcceleration

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

In order to be consistent with our current URL schemes, I think paths like
about:preferences/privacy
would make the most sense.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Use hash to select categories

Using hash instead of slash.

Philipp, is this change OK with you? We will use about:preferences#privacy instead of about:preferences/privacy.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Use friendly names for categories

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=dfbbe76b52ea

Comment 6

[:Gijs (he/him)]

Comment on attachment 8438670 [details] [diff] [review]
Use hash to select categories

Clearing review per discussion on IRC.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8438670 [details] [diff] [review]
Use hash to select categories

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

::: browser/components/preferences/in-content/preferences.js
@@ +60,1 @@
>    window.history.replaceState(page, document.title);

Have you tried the following instead?
window.history.replaceState(page, document.title, window.location.hash);

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8438670 [details] [diff] [review]
Use hash to select categories

Looks good!
The # is fine since those URLs are unlikely to be typed in directly and they are still readable.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Use hash to select categories (v2)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Use friendly names for categories (v2)

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=f7953ab1d354

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8439701 [details] [diff] [review]
Use friendly names for categories (v2)

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

::: browser/base/content/utilityOverlay.js
@@ +481,5 @@
>  
> +function internalPrefCategoryNameToFriendlyName(aName) {
> +  if (!aName)
> +    return "";
> +  return aName.substr(4).toLowerCase();

It seems like it would be good to check the prefix before blindly chopping the beginning off otherwise someone who forgets the "pane" prefix is going to end up with a weird anchor. For example, "general" is going to give about:preferences#ral. We may want to log an error if "pane" is not the prefix so that developers realize why the correct pane isn't being used.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Matthew N. [:MattN] from comment #12)

Yeah, that sounds like a good idea.

Comment 14

[Dão Gottwald [:dao]]

Please do not declare internalPrefCategoryNameToFriendlyName in the global scope, as you don't expect random utilityOverlay.js consumers (of which there are many) to call that function.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8439700 [details] [diff] [review]
Use hash to select categories (v2)

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

I really like where this is going, but the switchToTabHavingURI thing I think is something we need to sort out. :-(

::: browser/base/content/utilityOverlay.js
@@ +494,5 @@
>        return;
>      }
>  
> +    let preferencesURI = "about:preferences" + (paneID ? "#" + paneID : "");
> +    let newLoad = !win.switchToTabHavingURI(preferencesURI, true);

This is an interesting decision. It means that you can now open several tabs with different pages selected in each. Is that desirable?

I suspect not, and in particular, opening a new tab will redirect to about:preferences#general, but if something tries to open the prefs through here without a paneID, it will open a new instance because the URL is not the same (yet). I don't think that's what we want.

This is kind of annoying because AFAICT switchToTabHavingURI doesn't have a way to ignore the hash. We would need to add one.

::: browser/components/preferences/in-content/preferences.js
@@ +51,4 @@
>  }
>  
> +function onHashChange() {
> +  let hash = document.location.hash;

So if I:

1) load about:preferences
2) then get redirected to about:preferences#general
3) hit the back button

Doesn't that then break this code because location.hash is empty ? I expected this would need the same code as the init function above.

Generally, to reduce duplication, I wonder if it makes sense to push the hash reading code into gotoPref - aCategory can be determined there from the hash if it's not passed in directly by the consumer. Does that make sense at all?

@@ +61,5 @@
> +    return;
> +  gLastHash = aCategory;
> +  let categories = document.getElementById("categories");
> +  let item = categories.querySelector(".category[value=" + aCategory + "]");
> +  categories.selectedItem = item;

Do we want to guard this at all against non-existing items? How badly does this break if the user types after the hash and the item doesn't exist? (e.g. "synca")

Could just default to paneGeneral if item is null on the line above, maybe?

@@ +63,5 @@
> +  let categories = document.getElementById("categories");
> +  let item = categories.querySelector(".category[value=" + aCategory + "]");
> +  categories.selectedItem = item;
> +  document.location.hash = aCategory;
> +  window.history.replaceState(aCategory, document.title);

After Matt's earlier suggestion, I'm confused... what does this do? :-)

Comment 16

[:Gijs (he/him)]

Comment on attachment 8439701 [details] [diff] [review]
Use friendly names for categories (v2)

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

I think this part is solid, but the work required on the old part will mean this patch no longer applies easily, so you may as well re-request review on both parts if you wanted to do that. I could also live with them just being one patch, I think, although I do appreciate that you took the time to split them into logical bits. :-)

::: browser/base/content/utilityOverlay.js
@@ +479,5 @@
>    window.openDialog("chrome://browser/content/aboutDialog.xul", "", features);
>  }
>  
> +function internalPrefCategoryNameToFriendlyName(aName) {
> +  if (!aName)

Concurring with MattN here, we should only chop stuff off if it starts with pane. In particular, I think this might be a workable nearly-one-liner:

return (aName || "").replace(/^pane./, function(toReplace) { return toReplace[4].toLowerCase(); });

In particular, this deals with the empty case, and only lowercases the first letter instead of the entire pane ID, and will do nothing if the text doesn't start with 'pane'.

::: browser/components/preferences/in-content/preferences.js
@@ +53,5 @@
>  
>  function onHashChange() {
>    let hash = document.location.hash;
> +  let category = hash.substr(1) || "general";
> +  category = friendlyPrefCategoryNameToInternalName(category);

This lends more weight to my argument that we should push this into gotoPref. ;-)

@@ +64,5 @@
>    let categories = document.getElementById("categories");
>    let item = categories.querySelector(".category[value=" + aCategory + "]");
>    categories.selectedItem = item;
> +  let hash = internalPrefCategoryNameToFriendlyName(aCategory);
> +  if (gLastHash || aCategory != "paneGeneral") {

Oh. So I guess this works around at least the pathological "infinite general pane about:preference tabs" case that I was worried about wrt switchToTabHavingURI. Still, in the ideal case, if you had prefs open on pane foo, and someone said "open prefs on pane bar", it would reuse the same prefs instance. That's certainly what it did before.

Note also that I think the easier way to do this might be:

document.location.hash = (aCategory == "paneGeneral") ? "" : aCategory;

Which would also take care of switching to another pane and then back to the general one. But maybe I'm missing some reason why that wouldn't work?

I guess with the pathological case fixed, if you want to followup the general one, that would wfm, but ideally I think we should address it here.

@@ +88,5 @@
>                              .getAttribute("helpTopic");
>    openHelpLink(helpTopic);
>  }
> +
> +function friendlyPrefCategoryNameToInternalName(aName) {

Nit: I think this should return aName as-is if aName.startsWith("pane").

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8439701 [details] [diff] [review]
Use friendly names for categories (v2)

see comment 14

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #16)
> Note also that I think the easier way to do this might be:
> 
> document.location.hash = (aCategory == "paneGeneral") ? "" : aCategory;
> 
> Which would also take care of switching to another pane and then back to the
> general one. But maybe I'm missing some reason why that wouldn't work?

I didn't do this because setting document.location.hash = "" will change the location to have the # character appended to it. For example, about:preferences -> about:preferences#

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #15)
> @@ +63,5 @@
> > +  let categories = document.getElementById("categories");
> > +  let item = categories.querySelector(".category[value=" + aCategory + "]");
> > +  categories.selectedItem = item;
> > +  document.location.hash = aCategory;
> > +  window.history.replaceState(aCategory, document.title);
> 
> After Matt's earlier suggestion, I'm confused... what does this do? :-)

Using replaceState as MattN suggested will place the third-argument after the location, like about:preferences/privacy, which doesn't work with how we have about: URIs setup.

Comment 20

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> (In reply to :Gijs Kruitbosch from comment #15)
> > @@ +63,5 @@
> > > +  let categories = document.getElementById("categories");
> > > +  let item = categories.querySelector(".category[value=" + aCategory + "]");
> > > +  categories.selectedItem = item;
> > > +  document.location.hash = aCategory;
> > > +  window.history.replaceState(aCategory, document.title);
> > 
> > After Matt's earlier suggestion, I'm confused... what does this do? :-)
> 
> Using replaceState as MattN suggested will place the third-argument after
> the location, like about:preferences/privacy, which doesn't work with how we
> have about: URIs setup.

I believe it would be similar to |document.location.hash = aCategory;| if the argument starts with "#" like in my example. I'm not sure if the hashchange event would still fire or whether you need it to or not.

Comment 21

[Matthew N. [:MattN]]

Please keep sub-panes e.g "#advanced/network" in mind during implementation if you're not going to implement that here.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: patch wip

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (needs to apply on top of bug 1025195)

Let's not handle the advanced subpane navigation in this bug. We want to move away from using the tabpanels in the preferences (impossible to show multiple tabpanels with search) and it adds more complexity for a feature that isn't present in any category except for Advanced.

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8441681 [details] [diff] [review]
Patch (needs to apply on top of bug 1025195)

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

r=me with comments and follow-up suggestions.

::: browser/base/content/utilityOverlay.js
@@ +501,5 @@
>  
> +    let friendlyCategoryName = internalPrefCategoryNameToFriendlyName(paneID);
> +    let preferencesURI = "about:preferences" +
> +                         (friendlyCategoryName ? "#" + friendlyCategoryName : "");
> +    let newLoad = !win.switchToTabHavingURI(preferencesURI, true, {ignoreFragment: true});

s/preferencesURI/preferencesURL/

We try to use "URL" when it's a string and "URI" when referring to an nsIURI.

::: browser/components/preferences/in-content/preferences.js
@@ +44,5 @@
> +  window.addEventListener("hashchange", onHashChange);
> +  gotoPref();
> +
> +  var initFinished = document.createEvent("Event");
> +  initFinished.initEvent("Initialized", true, true);

Nit: FYI initEvent is deprecated. Since you're just moving this, you can leave it if you want.

@@ +48,5 @@
> +  initFinished.initEvent("Initialized", true, true);
> +  document.dispatchEvent(initFinished);
> +  // Wait until initialization of all preferences are complete before
> +  // notifying observers that the UI is now ready.
> +  Services.obs.notifyObservers(window, "advanced-pane-loaded", null);

It seems like we should remove the advanced-pane-loaded notification since it's not specific to the advanced pane anymore and the event can be used instead. Can you file a follow-up? add-on MXR is showing no results but it seems like the search may be failing so I'm not sure.

@@ +57,5 @@
>  }
>  
> +function gotoPref(aCategory) {
> +  let hash = document.location.hash;
> +  let category = aCategory || hash.substr(1) || "general";

I would prefer if "general" was a constant or ideally looked up from the first tab. e.g. categories.firstElementChild.value or categories.querySelector(".category").value. Tests for the different behaviours of this function would be good. The function could also use more whitespace and/or comments.

@@ +64,5 @@
> +    return;
> +  let categories = document.getElementById("categories");
> +  let item = categories.querySelector(".category[value=" + category + "]");
> +  if (!item) {
> +    category = "paneGeneral";

Another hard-coded reference to general which could use the above cleanup.

@@ +68,5 @@
> +    category = "paneGeneral";
> +    item = categories.querySelector(".category[value=" + category + "]");
> +  }
> +  let newHash = internalPrefCategoryNameToFriendlyName(category);
> +  if (gLastHash || category != "paneGeneral") {

And again

@@ +73,5 @@
> +    document.location.hash = newHash;
> +  }
> +  gLastHash = category;
> +  categories.selectedItem = item;
> +  window.history.replaceState(category, document.title);

Side-note: IMO we shouldn't assume the only state we want to store is the category so I would prefer if we used an category was just a property on a new object. It'd be good to fix this before it gets relied upon e.g. in tests or too much code. Theoretical Example:
{
  searchQuery: "proxy"
  category: "advanced"
  subCategory: "networking"
}
This can also be a follow-up if you prefer.

@@ +94,5 @@
>    openHelpLink(helpTopic);
>  }
> +
> +function friendlyPrefCategoryNameToInternalName(aName) {
> +  if (aName.startsWith("pane"))

Would it be hard to replace all internal uses of the internal name and only support them for backwards compat for selecting a pane? Then we could simplify gotoPref a bit and remove this function. It doesn't need to be done in this bug though.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/6e54974c922c

(In reply to Matthew N. [:MattN] from comment #24)
> Nit: FYI initEvent is deprecated. Since you're just moving this, you can
> leave it if you want.

Filed bug 1027819.
 
> It seems like we should remove the advanced-pane-loaded notification since
> it's not specific to the advanced pane anymore and the event can be used
> instead. Can you file a follow-up? add-on MXR is showing no results but it
> seems like the search may be failing so I'm not sure.

Filed bug 1027821.

> Side-note: IMO we shouldn't assume the only state we want to store is the
> category so I would prefer if we used an category was just a property on a
> new object. It'd be good to fix this before it gets relied upon e.g. in
> tests or too much code.

There are a number of tests that check this state object already. I filed bug 1027822 as a follow-up for this.

> Would it be hard to replace all internal uses of the internal name and only
> support them for backwards compat for selecting a pane? Then we could
> simplify gotoPref a bit and remove this function. It doesn't need to be done
> in this bug though.

I originally started to do this in attachment 8438671 [details] [diff] [review] but it became quite a giant patch. I filed bug 1027827 for this.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Follow-up changeset to update the switchToTabHavingURI call to use the API that was changed in the review process of bug 1025195.

https://hg.mozilla.org/integration/fx-team/rev/4d27c150d958

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e54974c922c
https://hg.mozilla.org/mozilla-central/rev/4d27c150d958

Comment 28

[Marco Mucci [:MarcoM]]

Hi Florin, this bug requires a QA contact for verification.  The desktop iteration ends on Monday, is it possible to have it verified by then?

Comment 29

[Camelia Badau [:cbadau], Desktop QA]

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using latest Nightly 33.0a1 (buildID: 	20140622030203) and I have the following comments:  
- if I open about:preferences#general, the options favicon is displayed, but if I open about:preferences#content(applications, sync, privacy, security, advanced) the options favicon is not displayed -> please see screenshot "A.PNG"
- I can open several tabs with different pages selected in each -> is this intended behaviour? 
- if I write for example about:preferences#appl, I am redirect to the General tab -> is this intended behaviour?

Comment 30

[Camelia Badau [:cbadau], Desktop QA]

Attachment: A.png

Comment 31

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Camelia Badau, QA [:cbadau] from comment #29)
> - I can open several tabs with different pages selected in each -> is this
> intended behaviour?
> - if I write for example about:preferences#appl, I am redirect to the
> General tab -> is this intended behaviour?

Both of these sound good from a UX perspective.
The icon should obviously be displayed regardless of the selected category.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Yes, please file a follow-up bug for the missing favicon. The other two notes are intentional and are by design.

Comment 33

[Camelia Badau [:cbadau], Desktop QA]

I filled bug 1028921 for the missing favicon.

Comment 34

[Carlos [nohamelin]]

I'm seeing that chrome overlays defined for about:preferences aren't being applied(?) if about:preferences#general or any other similar url is used for opening the page. It will affect to add-ons: I noted it with one of mine (Simple Locale Switcher); I remember a few other extensions doing it too (tabs-related add-ons exposing its own stuff).

I don't know yet the right way for to manage this change, or if I'm missing something: overlaying any possible url seems to work but it isn't a sane option...

Comment 35

[:Gijs (he/him)]

(In reply to Carlos [:nohamelin] from comment #34)
> I'm seeing that chrome overlays defined for about:preferences aren't being
> applied(?) if about:preferences#general or any other similar url is used for
> opening the page. It will affect to add-ons: I noted it with one of mine
> (Simple Locale Switcher); I remember a few other extensions doing it too
> (tabs-related add-ons exposing its own stuff).
> 
> I don't know yet the right way for to manage this change, or if I'm missing
> something: overlaying any possible url seems to work but it isn't a sane
> option...

Please file a separate bug to have XUL overlays ignore the hash portion of the URL.