595356 - Replace "about:home" with "Firefox Start Page" in preferences dialog

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

bug 590877 made "about:home" the default homepage URI for Firefox

In the preferences window, we should display "about:home" as "Firefox Start", instead, which is more human readable.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Sorry to file this so late, but pretty important polish IMO.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Shorlander points out that this should also be the string used for the tooltip when you hover over the "Default" button.

Marco points out that if we do this, we'll have to add some checking to ensure that if the user edits the value, they end up with a properly formed URI or set of URIs. I propose that in cases where the user edits and leaves a bad value, we just revert to about:home for them :)

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Marco is sick today, so if someone's bored and wants to take this, feel free!

Comment 4

[Marco Bonardo [:mak]]

I have a wip patch actually, so it could be attached in an hour or so...

Comment 5

[Marco Bonardo [:mak]]

Attachment: pref part v1.0

this is the pref part... I thought of various ways like using stack or decks to show a fake inputfield, but this looks cleaner based on prefwindow APIs.

I don't have a patch for the home button tooltip yet, but I think having them separate and getting feedback earlier is better.
Feel free to make a modified patch from this as well, since I'm not 100% active today.

Comment 6

[Marco Bonardo [:mak]]

Attachment: tooltip part v1.0

and this could be the tooltip part.

With this I guess I'm givin up for today, hope it will be better tomorrow

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 474786 [details] [diff] [review]
tooltip part v1.0

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>@@ -2979,8 +2979,7 @@ function openHomeDialog(aURL)

>-      var homeButton = document.getElementById("home-button");
>-      homeButton.setAttribute("tooltiptext", aURL);
>+      gHomeButton.updateTooltip();

I think you can just remove this entirely - the pref observer already handles updating the tooltip text.

>+      if (homePage == "about:home")

Hmm, was going to suggest comparing to the default pref value... But I guess that might not always be correct if someone ships a different default home page, so maybe this is better off as-is.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Given that we're re-using the existing about:home string, I think this may not actually be [strings]...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: alternative patch

I was thinking we could just do something like this, which is somewhat simpler than your patch (though maybe not as user-friendly?).

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> Created attachment 475168 [details] [diff] [review]
> alternative patch
> 
> I was thinking we could just do something like this, which is somewhat simpler
> than your patch (though maybe not as user-friendly?).

well it's pretty bad for the user because when he will start to edit that, he will end up editing something like "Firefox Sta" and he could end up saving that as homepage. I did the onfocus/onblur changes exactly to avoid putting them in a situation that could confuse them on what to do.

Comment 11

[Marco Bonardo [:mak]]

and at that point we should handle that case as Mike suggested, checking if the value is a valid url. but that is again tricky because that field can also contain multiple urls separated by "|".
And if we change the title of the page, we change the text the user has to enter. in syncToHomePref you compare to aboutHomeTitle but that's different based on the fact this is minefield, firefox or some other codename.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #7)
> Comment on attachment 474786 [details] [diff] [review]
> tooltip part v1.0
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >@@ -2979,8 +2979,7 @@ function openHomeDialog(aURL)
> 
> >-      var homeButton = document.getElementById("home-button");
> >-      homeButton.setAttribute("tooltiptext", aURL);
> >+      gHomeButton.updateTooltip();
> 
> I think you can just remove this entirely - the pref observer already handles
> updating the tooltip text.

you are absolutely right, gHomeButton object does this by itself.

> >+      if (homePage == "about:home")
> 
> Hmm, was going to suggest comparing to the default pref value... But I guess
> that might not always be correct if someone ships a different default home
> page, so maybe this is better off as-is.

I think this code is too bound to our dtd and behavior to replace all default homepages to aboutHomeTitle string :)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> well it's pretty bad for the user because when he will start to edit that, he
> will end up editing something like "Firefox Sta" and he could end up saving
> that as homepage.

It was already possible to enter invalid home page values - this only makes things slightly worse, because the field starts out containing non-URL text (which may tempt them to insert some other non-magical-non-URL string in the hopes of it working). If they do get into that state they can easily revert to default with the button, or just fix the entry manually again, though, so I'm not sure that's a blocking issue.

(In reply to comment #11)
> And if we change the title of the page, we change the text the user has to
> enter.

I don't think that's a problem - we're unlikely to change the title very often, and even if we do, we're not really trying to support the user entering this string manually, since there's a button that can be used to achieve the same effect. I included the onSyncToPreference just because it was easy to do.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> I don't think that's a problem - we're unlikely to change the title very often,

it's a problem at least when moving between minefield and firefox for example, where titles are different
I'm not sure, it looks really unpolished to me without the focus/blur change, like if we were unable to make a temporary change and we just put some text in the field.

Comment 15

[Alex Limi (:limi) — Firefox UX Team]

The UI here is really broken, but I guess we could do something like the following, which isn't too bad:* Default to "Firefox Start" as the value in the field, rendered as placeholder (gray) text.* If you focus the field and start typing, that will replace the placeholder text.* If you delete the string in the field, it reverts to "Firefox Start" as the placeholder text.* If you really want a blank page as your startup page, you would have to know that about:blank exists, but I really think this is an acceptable compromise, since there is a "Show a blank page" in the pulldown above ("When Firefox starts…[  |v] ).That way, Firefox Start is the implicit value of the field if you don't specify anything.I'd really like us to avoid having "magical strings" that mean a certain thing here. :)

Comment 16

[Alex Limi (:limi) — Firefox UX Team]

Sorry, bugzilla broke my formatting :(

Again:

The UI here is really broken, but I guess we could do something like the
following, which isn't too bad:

* Default to "Firefox Start" as the value in the field, rendered as placeholder
(gray) text.

* If you focus the field and start typing, that will replace the placeholder
text.

* If you delete the string in the field, it reverts to "Firefox Start" as the
placeholder text.

* If you really want a blank page as your startup page, you would have to know
that about:blank exists, but I really think this is an acceptable compromise,
since there is a "Show a blank page" in the pulldown above ("When Firefox
starts…[  |v] ).

That way, Firefox Start is the implicit value of the field if you don't specify
anything.

I'd really like us to avoid having "magical strings" that mean a certain thing
here. :)

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)> it's a problem at least when moving between minefield and firefox for example,> where titles are differentWhy? The pref value remains about:home before and after, it's just the string that appears in the textbox that changes depending on the build you're running. "Reset to default" keeps working too. The only thing that doesn't work is manually entering "Firefox Start Page" in Minefield, and what I was trying to say in the second part of comment 13 is that we don't really care to support that.

Comment 18

[Marco Bonardo [:mak]]

oh and yes, this is not [strings] since we can reuse the text.

Comment 19

[Marco Bonardo [:mak]]

I still don't like the fact users have to type "Firefox Start Page" magic text in a url (plus weird "|" char) field (Apropos, you should compare lowercased values), that text changes based on locale and branding.

Limi's suggestion to use placeholder could work, we could even ideally remove the "restore default" button if users get used to clear the field to use default. It would practically work like my patch but without the focus/blur stuff.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch for limi's suggestion (use placeholder text)

This turned out to be not so difficult... this patch:
- shows the about:home title as placeholder text if your home page is about:home
- if you click "reset to default", or empty the textbox manually, the home page is set to "about:home" and the textbox is empty (shows placeholder text)
- supports existing users that have the pref set to "" by showing about:blank for that case

This makes it impossible to actually set the pref to "" (which we treat the same way as about:blank) in the pref dialog, but I don't think that's really an issue.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch for limi's suggestion (use placeholder text, cleaned up)

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)
> I still don't like the fact users have to type "Firefox Start Page" magic text
> in a url (plus weird "|" char) field (Apropos, you should compare lowercased
> values), that text changes based on locale and branding.

There's no reason that they would ever have to type it, though. There's a button that does it for them.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Good catch re: case-insensitive compares - would need to roll that into the patch as well.

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 475280 [details] [diff] [review]
patch for limi's suggestion (use placeholder text, cleaned up)


>diff --git a/browser/components/preferences/main.js b/browser/components/preferences/main.js
>--- a/browser/components/preferences/main.js
>+++ b/browser/components/preferences/main.js
>@@ -69,16 +69,46 @@ var gMainPane = {
>    *     2: the last page the user visited (DEPRECATED)
>    *     3: windows and tabs from the last session (a.k.a. session restore)
>    *
>    *   The deprecated option is not exposed in UI; however, if the user has it
>    *   selected and doesn't change the UI for this preference, the deprecated
>    *   option is preserved.
>    */
> 
>+  syncFromHomePref: function (textbox)
>+  {
>+    let aboutHomeTitle = textbox.getAttribute("abouthometitle");

this does not exist anymore in the patch

>+    let homePref = document.getElementById("browser.startup.homepage");
>+
>+    // If the pref is set to about:home, set the value to "" to show the
>+    // placeholder text (about:home title)

end comment with period

>+    if (homePref.value == "about:home")
>+      return "";
>+
>+    // If the pref is actually "", show about:blank - the actual home page
>+    // loading code treats them the same, and we don't want the placeholder text
>+    // showing.

replace - with comma and double spacing
", and..." should probably be "but we don't want to show the placeholder text for it."

>+    if (homePref.value == "")
>+      return "about:blank";
>+
>+    // Else show the actual pref value

end with comma

>+    return undefined;
>+  },
>+
>+  syncToHomePref: function (textbox)
>+  {
>+    // If the value is "", use about:home

ditto

>+    if (textbox.value == "")
>+      return "about:home";
>+
>+    // Else use the actual textbox value

ditto

should be fine, will test it briefly but I like it

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 474786 [details] [diff] [review]
tooltip part v1.0

Split this off to bug 596409.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Comment 27

[Alex Limi (:limi) — Firefox UX Team]

Just to clarify; I didn't actually mean that the placeholder text should say "about:home", but that it should say "Firefox Start". What I didn't want was a "magical string" of any kind, but that probably wasn't what was proposed. :)

So, when there isn't any content in the string field, it says "Firefox Start" in the gray placeholder text.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #27)
> So, when there isn't any content in the string field, it says "Firefox Start"
> in the gray placeholder text.

Yes, that's what the patch implements.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 475301 [details] [diff] [review]
updated patch

Fine with alternate solution, yeah.

Comment 30

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ce5800998e6e

Comment 31

[u88484]

Verified fixed

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre