Closed Bug 1038996 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 verified, fennec+)

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- verified
fennec + ---

People

(Reporter: sawrubh, Assigned: jonalmeida, Mentored)

References

Details

(Keywords: ux-implementation-level, ux-jargon, Whiteboard: [fxgrowth][lang=java][good next bug])

Attachments

(3 files)

bug-1038996-patch-v1.patch
3.95 KB, patch
mfinkle
: feedback+
Details | Diff | Splinter Review
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
40 bytes, text/x-review-board-request
jonalmeida
: review+
Details
Screenshot_2015-09-18-11-40-34.png
90.25 KB, image/png
Details 
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.
No need to CC me, I see every bug. Also, I like this idea.
OS: Linux → Android
Hardware: x86_64 → ARM
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.
(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"
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) ?
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.
Assignee: nobody → ju.sanchez9
Status: NEW → ASSIGNED
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...
Attachment #8481857 - Flags: feedback?(nchen)
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.
Attachment #8481857 - Flags: feedback?(nchen) → feedback+
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..
Flags: needinfo?(mark.finkle)
My bad, wrong bug. ^
Flags: needinfo?(mark.finkle)
What happened here?
Julien are you still interested in working on this?
Flags: needinfo?(ju.sanchez9)
Yes, why not. Did the design team gave feedback on it ?
Flags: needinfo?(ju.sanchez9)
(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.
Let's include about:privatebrowsing too
Keywords: ux-implementation-level, ux-jargon
Summary: about:home on the new tab should be a placeholder → Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder
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.
Hey, Julien. mfinkle gave some feedback in comment 7 - is that enough to start working with?
Mentor: bnicholson
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.
Julien/Brian: possible to get this bug back into the dev queue?
Flags: needinfo?(ju.sanchez9)
Flags: needinfo?(bnicholson)
Whiteboard: [fxgrowth]
Nom'ing for triage.
Assignee: ju.sanchez9 → nobody
Mentor: bnicholson → michael.l.comella
Status: ASSIGNED → NEW
tracking-fennec: --- → ?
Flags: needinfo?(bnicholson)
Same jargon w/about:home on Firefox iOS
Yes, still ok for me, to work on it
Flags: needinfo?(ju.sanchez9)
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.
Flags: needinfo?(alam)
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.
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.
Flags: needinfo?(alam)
tracking-fennec: ? → +
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.
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.
(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.
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...
Flags: needinfo?(jalmeida)
Whiteboard: [fxgrowth] → [fxgrowth][lang=java][good next bug]
I've secretly already started working on it since Anthony approved the idea a few hours ago :)
Flags: needinfo?(jalmeida)
Assignee: nobody → jalmeida
Bug 1038996 - Don't prepopulate the address bar with 'about:home', and 'about:privatebrowsing' on new tabs; they should be a placeholder r?mcomella
Attachment #8641736 - Flags: review?(michael.l.comella)
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.
Attachment #8641736 - Flags: review?(michael.l.comella)
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 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
Attachment #8641736 - Flags: review?(michael.l.comella)
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 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 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();
Attachment #8641736 - Flags: review?(michael.l.comella) → review+
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 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
Attachment #8641736 - Flags: review+ → review?(michael.l.comella)
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 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!
Attachment #8641736 - Flags: review+
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
Attachment #8641736 - Flags: review+
Attachment #8641736 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
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)
Flags: needinfo?(jalmeida)
Depends on: 1192033
Attachment #8641736 - Flags: review+ → review?(michael.l.comella)
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
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 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.
Attachment #8641736 - Flags: review?(michael.l.comella) → review+
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
Attachment #8641736 - Flags: review+ → review?(michael.l.comella)
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.
Attachment #8641736 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
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() :)
Keywords: checkin-needed
https://reviewboard.mozilla.org/r/14595/#review13893

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

Thanks! Removed more unnecessary code :)
Keywords: checkin-needed
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
Attachment #8641736 - Flags: review+ → review?(michael.l.comella)
Attachment #8641736 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/e7d7cc7dbd04
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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?
(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! :)
Sounds good. Bug 1193398.
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.
Blocks: 1208512
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: