1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Saurabh Anand [:sawrubh]]

STR:
* Open a New Tab.
* Tap on the Awesomebar.

What happens:
`about:home` is selected as the text in the awesomebar.

What I expect:
`about:home` should be a placeholder instead of actual text. Sometimes I tap on the awesomebar and realize that I want to paste a url. In order to do that, I first need to delete this text (which is one backspace key tap but still). I see no benefit of not having it as a placeholder. I know I should ideally have just Tap-and-Hold on the Awesomebar to either do a 'Paste' or a 'Paste and Go'.

Additional Note:
We should also probably make it easier for the user to figure out that Tap and Hold on the Awesomebar works (unlike Chrome), probably using some UI hints or whatever.

Comment 1

[Aaron Train [:aaronmt]]

No need to CC me, I see every bug. Also, I like this idea.

Comment 2

[Chris More [:cmore]]

I +1 this change. I was just about to file this same bug as this has been an annoyance when searching the awesomebar on Android. I don't see the purpose of displaying about:home in the awesomebar especially when it says "Enter search or Address" and then it shows about:home when tapped. I understand technically that it is an in-product page, but to an end user, it doesn't provide value. On Firefox desktop, if you open about:home or about:newtab, the awesomebar says "Search or enter address" and when selected, there is no text. For consistency and improved UX, this change seems like a win. I would just remove about:home from the address bar and allow users to search.

Comment 3

[Chris More [:cmore]]

(In reply to Saurabh Anand [:sawrubh] from comment #0)
> STR:
> * Open a New Tab.
> * Tap on the Awesomebar.
> 
> What happens:
> `about:home` is selected as the text in the awesomebar.
> 
> What I expect:
> `about:home` should be a placeholder instead of actual text. Sometimes I tap
> on the awesomebar and realize that I want to paste a url. In order to do
> that, I first need to delete this text (which is one backspace key tap but
> still). I see no benefit of not having it as a placeholder. I know I should
> ideally have just Tap-and-Hold on the Awesomebar to either do a 'Paste' or a
> 'Paste and Go'.
> 
> Additional Note:
> We should also probably make it easier for the user to figure out that Tap
> and Hold on the Awesomebar works (unlike Chrome), probably using some UI
> hints or whatever.

Do you think we should change the summary to be a bit more explicit? Maybe:

"Remove about:home text from awesomebar when tapped to search or enter address"

Comment 4

[Julien Sanchez]

Ok for me to work on it. Can you assign it to me ?

If I summarize, the solution is to clear the "about:home" when the user click on the address bar (awesomebar) ?

Comment 5

[Aaron Train [:aaronmt]]

You're welcome to take the bug, however I don't know if the design team has seen this to comment on. I have added them to this bug to determine if they would like this change.

Comment 6

[Julien Sanchez]

Attachment: bug-1038996-patch-v1.patch

Here a first version of the patch. Just listen for touch event on the editText, and clear the text on touch, if the value is "about:home"

Waiting the response of the design team...

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8481857 [details] [diff] [review]
bug-1038996-patch-v1.patch

This patch might be a good start. Desktop has a larger set of pages though:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#183

These pages all get set to an empty string:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2205

The MultiProcessBrowser check is for e10s and is not supported in Fennec. The | content.opener | check might be harder to support with our Java/Gecko separation.

Also, bnicholson might be a good reviewer/mentor for this bug. Nothing against jchen (I love him) but I don't know how familiar he is thing this code area and Desktop's behavior.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I see work is picking up again on this bug. Mark, is the old icon still good for now? (for the notification bar). It seems a bit dated and I've forgotten where we got that from now..

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

My bad, wrong bug. ^

Comment 10

[Aaron Train [:aaronmt]]

What happened here?

Comment 11

[Aaron Train [:aaronmt]]

Julien are you still interested in working on this?

Comment 12

[Julien Sanchez]

Yes, why not. Did the design team gave feedback on it ?

Comment 13

[Aaron Train [:aaronmt]]

(In reply to Julien Sanchez from comment #12)
> Yes, why not. Did the design team gave feedback on it ?

Nope, was just querying up open stagnant bugs with patches attached.

Comment 15

[Aaron Train [:aaronmt]]

Let's include about:privatebrowsing too

Comment 16

[John Karahalis [:openjck]]

I'll go further and say that the terms about:home and about:private should be removed from the interface completely. The terms are meaningless to average users and expert users who need to know about them already do, or can easily look them up.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Julien. mfinkle gave some feedback in comment 7 - is that enough to start working with?

Comment 18

[Chris More [:cmore]]

Any movement here?

I would recommend about:* should never be real text on mobile. We don't do it on desktop, we shouldn't surely do it on mobile where search is a very common UX feature.

Comment 19

[Chris More [:cmore]]

Julien/Brian: possible to get this bug back into the dev queue?

Comment 20

[Brian Nicholson (:bnicholson)]

Nom'ing for triage.

Comment 21

[Aaron Train [:aaronmt]]

Same jargon w/about:home on Firefox iOS

Comment 22

[Julien Sanchez]

Yes, still ok for me, to work on it

Comment 25

[:Margaret Leibovic]

Based on the discussion in bug 1180908, I'm not sure whether or not we want to actually do this.

antlam, have you given this more thought?

I'd be happy to WONTFIX this.

Comment 26

[John Karahalis [:openjck]]

As someone who submitted a duplicate, I found the points in bug 1180908 to be interesting. I can definitely see cases where the feature would be useful.

I think part of the puzzle revolves around how often the feature provides value relative to how often it causes confusion. To an average user who is scared of jargon and who doesn't usually talk with others about the software they use, I worry the feature might do more harm than good.

Comment 27

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

After thinking about this some more, perhaps the user education aspect has already served it's purpose. We can remove the highlighted "about:home" text to visually simplify this for our users.

I think the real benefit to removing this is for general users that don't necessarily use about:* pages via typing them in our URL bar. For our more "power users" I think we've already left it in there long enough for them and if visibility is the main concern there's probably better ways to do that.

Let's go ahead and remove the highlighted text when a user presses the URL bar to type. We can keep them as placeholders where we currently don't say "Search or enter address". This should have the added benefit of keeping "Search or enter address" around a bit longer for users looking to use the awesomebar.

Comment 28

[Chris More [:cmore]]

My thoughts is if the search bar says "Search or enter address" and you click it and it says "about:home", that's confusing. Removing friction for common use cases (like searching) in browsers should be good for general audiences and power users. We don't show about:home or about:newtab on desktop when the awesome bar is in focus when you click it and I don't see why we wouldn't follow the same pattern on mobile especially when there is only one search field available.

Comment 29

[Chris More [:cmore]]

what's the specific user story about when having about:home as real text is useful to a user segment? Trying to understand the use case on how the current UX is ideal for some type of user.

Comment 30

[deadrat]

(In reply to Chris More [:cmore] from comment #29)
> what's the specific user story about when having about:home as real text is
> useful to a user segment? Trying to understand the use case on how the
> current UX is ideal for some type of user.

Hi,  I am just a user of Firefox. My story is that this text pre populating the awesome bar is quite annoying. I didn't search for this bug using right keyword, so I ended up filling a duplicate bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1187458 . 

It is hard to imagine how this might be useful for someone. Even if it's useful, I guess they might be minority. Majority of the users may want the field to be clear so that they can search or go to a location easily.

Comment 31

[:Margaret Leibovic]

I don't think the current behavior is there because it's "useful", I think it's there because it's the default behavior, and we never decided to do anything about changing it.

So, let's change it! We just need someone to pick up work on this. Maybe Jonathan would be interested...

Comment 32

[Jonathan Almeida [:jonalmeida]]

I've secretly already started working on it since Anthony approved the idea a few hours ago :)

Comment 33

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd7db14f1603

Comment 34

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=306db11e07c8

Comment 35

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

https://reviewboard.mozilla.org/r/14595/#review13229

::: mobile/android/base/toolbar/ToolbarEditText.java:111
(Diff revision 1)
> +            // See bug 1038996

There are two schools of thought on specifying a bug number in comments (don't, b/c it's in blame, and do) and I'm stuck in between them: I find it useful to include a bug number when there's an absolutely horrible hack that you need to look up the bug to understand and since blame can change, it's nice to leave the bug number in. Otherwise, I leave it out.

So I'd leave it out here. :) But your call.

::: mobile/android/base/toolbar/ToolbarEditText.java:113
(Diff revision 1)
> +                    getText().toString().equals(AboutPages.PRIVATEBROWSING)) {

When you repeat a function call several times in an if statement, it's good to store it as a temporary variable:
  1) If you happen to screw up the function calls the second time, you're not going to get the same results. This is especially relevant when changing the code, where you have to change every instance. e.g. this is also one of the reasons for specifying constants in code.
  2) Function calls are less efficient than storing the result as a local variable (I think). In particular, you specify .toString() which is likely to allocate a new String Object and we'd like to avoid allocations. In any case, we can always pray the compiler does the right thing, in which case I defer to 1). :P

::: mobile/android/base/toolbar/ToolbarEditText.java:112
(Diff revision 1)
> +            if (getText().toString().equals(AboutPages.HOME) ||

You can use AboutPages.isAboutHome here (and should probably write one for private browsing).

::: mobile/android/base/toolbar/ToolbarEditText.java:114
(Diff revision 1)
> +                setText("");

Why do you do this in onFocusChanged? I would imagine this would happen in setText, at their callsites, or similar.

Comment 37

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4d0a5a2695

Comment 38

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/14595/#review13229

> Why do you do this in onFocusChanged? I would imagine this would happen in setText, at their callsites, or similar.

Setting it onFocusChanged seemed reasonable since the only time we don't want to see the "about" text is when the user changes focus to the bar which is when the check should be made. setText() works as well and I guess it's fine if we go through that check there (although won't we hit that check more often that we need it to? Minor nit of course).

> When you repeat a function call several times in an if statement, it's good to store it as a temporary variable:
>   1) If you happen to screw up the function calls the second time, you're not going to get the same results. This is especially relevant when changing the code, where you have to change every instance. e.g. this is also one of the reasons for specifying constants in code.
>   2) Function calls are less efficient than storing the result as a local variable (I think). In particular, you specify .toString() which is likely to allocate a new String Object and we'd like to avoid allocations. In any case, we can always pray the compiler does the right thing, in which case I defer to 1). :P

I really like both points; fixed!

Java needs immutability by default. Will create bug for Oracle :P

Comment 39

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 40

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8074e51b5d11

Comment 41

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b9daeb6829

Comment 42

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=960f568ebcc2

Comment 43

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bba8eb536c8

Comment 44

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 45

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad216104e0f

Comment 46

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

https://reviewboard.mozilla.org/r/14595/#review13627

Ship It!

::: mobile/android/base/toolbar/ToolbarEditText.java:129
(Diff revision 4)
> +        if (AboutPages.isAboutHome(currentUrl) || AboutPages.isPrivateHome(currentUrl)) {

nit: Rather than duplicate the code we need to run (i.e. setText & resetAutocompleteState), you can set the text in a local variable and call the code once, i.e.:

`final CharSequence finalText; 
`if (isAboutHomeStuff...) {
`  finalText = "";
`} else {
`  finalText = text;
`}

`super.setText(finalText, type);
`resetAutocompleteState();

Comment 48

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/14595/#review13627

> nit: Rather than duplicate the code we need to run (i.e. setText & resetAutocompleteState), you can set the text in a local variable and call the code once, i.e.:
> 
> `final CharSequence finalText; 
> `if (isAboutHomeStuff...) {
> `  finalText = "";
> `} else {
> `  finalText = text;
> `}
> 
> `super.setText(finalText, type);
> `resetAutocompleteState();

Slightly better formatting... ¯\\_(ツ)_/¯

`final CharSequence finalText;`
`if (isAboutHomeStuff...) {`
`  finalText = "";`
`} else {`
`  finalText = text;`
`}`

`super.setText(finalText, type);`
`resetAutocompleteState();`

Comment 49

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ca8921a5edf

Comment 50

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 51

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/14595/#review13627

> Slightly better formatting... ¯\\_(ツ)_/¯
> 
> `final CharSequence finalText;`
> `if (isAboutHomeStuff...) {`
> `  finalText = "";`
> `} else {`
> `  finalText = text;`
> `}`
> 
> `super.setText(finalText, type);`
> `resetAutocompleteState();`

Fixed nits.

Comment 52

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

https://reviewboard.mozilla.org/r/14595/#review13649

Ship It!

Comment 53

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

https://reviewboard.mozilla.org/r/14595/#review13651

Comment 54

[Pulsebot]

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

Comment 55

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

Backed out in https://hg.mozilla.org/integration/fx-team/rev/97aba112e4a2 at bc's request.

Comment 56

[Jonathan Almeida [:jonalmeida]]

Backed out because of null pointer exceptions. Still trying to figure out how we get to this state..

java.lang.NullPointerException
	at org.mozilla.gecko.toolbar.ToolbarEditText.setText(ToolbarEditText.java:128)
	at android.widget.TextView.setText(TextView.java:3529)
	at org.mozilla.gecko.toolbar.ToolbarEditLayout.setText(ToolbarEditLayout.java:204)
	at org.mozilla.gecko.toolbar.BrowserToolbar.onTabChanged(BrowserToolbar.java:513)
	at org.mozilla.gecko.Tabs$5.run(Tabs.java:672)
	at android.os.Handler.handleCallback(Handler.java:725)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5041)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:793)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:560)
	at dalvik.system.NativeStart.main(Native Method)

Comment 57

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 58

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/14595/#review13701

::: mobile/android/base/toolbar/ToolbarEditText.java:127
(Diff revisions 5 - 6)
> +        final String textString = (text == null) ? "" : text.toString();

So we sometimes get a null CharSequence sent to setText and this is handled android by making it an empty CharSequence (i.e. ""). Since we put super.setText below we hit exception via the [autophone build system](https://treeherder.mozilla.org/#/jobs?filter-searchStr=autophone&exclusion_profile=false&repo=fx-team&revision=c9b41f67d832), so I've added the null check before calling toString.

Comment 59

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

https://reviewboard.mozilla.org/r/14595/#review13829

Ship It!

::: mobile/android/base/toolbar/ToolbarEditText.java:126
(Diff revision 6)
>  

nit: I'm not a fan of this empty line.

Comment 60

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 61

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Fixed nit.

Comment 62

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/14595/#review13893

::: mobile/android/base/AboutPages.java:49
(Diff revision 7)
> +    public static final boolean isPrivateHome(final String url) {

Btw. this looks like a method I just recently landed: AboutPages.isAboutPrivateBrowsing() :)

Comment 63

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82968e016745

Comment 64

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/14595/#review13893

> Btw. this looks like a method I just recently landed: AboutPages.isAboutPrivateBrowsing() :)

Thanks! Removed more unnecessary code :)

Comment 65

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8641736 [details]
MozReview Request: Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella

Comment 66

[Pulsebot]

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

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7d7cc7dbd04

Comment 68

[John Karahalis [:openjck]]

Awesome!

It may make sense to also adjust this behavior on Firefox for iOS. Should someone open a bug with them (I volunteer) or do they already watch/mirror changes to Firefox for Android?

Comment 69

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to John Karahalis [:openjck] from comment #68)
> It may make sense to also adjust this behavior on Firefox for iOS. Should
> someone open a bug with them (I volunteer) or do they already watch/mirror
> changes to Firefox for Android?

Some of the iOS developers do watch Android mail go by informally, but it's also possible for them to miss this bug – go ahead and file. Thanks for being on top of things! :)

Comment 70

[John Karahalis [:openjck]]

Sounds good. Bug 1193398.

Comment 71

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-09-18-11-40-34.png

Verified as fixed using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 43.0a1 (2015-09-17)

Opening a new tab and tapping on the URL Bar, "Search or enter address" text is displayed and it is not selected.