If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

create a pref that will show the full URL in the awesomebar instead of the title

VERIFIED FIXED in Firefox 23

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: imelven, Assigned: wesj)

Tracking

Trunk
Firefox 23
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox23 verified, relnote-firefox 23+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
A security researcher friend of mine asked me if there was a pref for this. This pref would be off by default but would let more advanced users always see the URL in the awesomebar instead of the page title. Wesj has cooked up an addon to do this, by the way.
Blocks: 605206
Duplicate of this bug: 857361
(Assignee)

Comment 2

5 years ago
If we wanted to do this, rather than a pref I've thought about allowing users to flick the title up or down to flip between the title and url.
Or adding a "Show Address" / "Show Title" toggle in the URLBar long tap menu
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Or adding a "Show Address" / "Show Title" toggle in the URLBar long tap menu

If we do implement this, then I think we had better collect some usage data and see which users prefer.
(Assignee)

Comment 5

5 years ago
Created attachment 735020 [details] [diff] [review]
Patch v1

Adds a context menu item on the urlbar to toggle between showing the title and url. Stored as a java pref. Affects all tabs forever once its set.
Attachment #735020 - Flags: review?(mark.finkle)
Comment on attachment 735020 [details] [diff] [review]
Patch v1

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+        SharedPreferences settings = activity.getSharedPreferences(GeckoApp.PREFS_NAME, 0);
>+        settings.registerOnSharedPreferenceChangeListener(this);
>+        mShowUrl = settings.getBoolean(GeckoApp.PREFS_SHOW_URL, false);

I wonder if this causes StrictMode violations? Just about anything related to SharedPreferences does. If so, use a background thread.
Is a listener gettign all pref changes? Will that cause pref problems?

>                 if (Tabs.getInstance().isSelectedTab(tab)) {
>-                    setTitle(tab.getDisplayTitle());
>+                    setTitle(tab.getDisplayTitle(mShowUrl));

Let's not make any Tab.getDisplayTitle changes. Instead:

setTitle(mShowUrl ? tab.getUrl() : tab.getDisplayTitle());

(or similar)

>-                    setTitle(tab.getDisplayTitle());
>+                    setTitle(tab.getDisplayTitle(mShowUrl));

Same

>     public void refresh() {

>             String url = tab.getURL();
>-            setTitle(tab.getDisplayTitle());
>+            setTitle(tab.getDisplayTitle(mShowUrl));

Same


>+    public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
>+        if (key.equals(GeckoApp.PREFS_SHOW_URL)) {
>+            mShowUrl = sharedPreferences.getBoolean(key, false);
>+            refresh();

refresh is not lightweight. Can't we just change the title? just call setTitle here?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>         GeckoAppShell.openUriExternal(url, "text/plain", "", "",
>-                                      Intent.ACTION_SEND, tab.getDisplayTitle());
>+                                      Intent.ACTION_SEND, tab.getDisplayTitle(false));

Another reason why we do not want to change Tab.getDisplayTitle

>+                ThreadUtils.postToBackgroundThread(new Runnable() {
>+                    @Override
>+                    public void run() {
>+                        SharedPreferences settings = getSharedPreferences(PREFS_NAME, 0);
>+                        SharedPreferences.Editor editor = settings.edit();
>+                        editor.putBoolean(PREFS_SHOW_URL, item.getItemId() == R.id.show_url);
>+                        editor.commit();
>+                    }
>+                });

I do wonder about the performance implications with using a listener

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_show_url "Show URL">
>+<!ENTITY contextmenu_show_title "Show Title">

"Display Address"
"Display Title"

?

I almost think that is too little caption. Maybe

"Display Address in Toolbar"
"Display Title in Toolbar"
Attachment #735020 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 735280 [details] [diff] [review]
Patch v2

I'm coming around to this.

> I wonder if this causes StrictMode violations? Just about anything related
> to SharedPreferences does. If so, use a background thread.
> Is a listener gettign all pref changes? Will that cause pref problems?
The listener only gets changes to the pref type we registerd for. I created a set of BrowserToolbarPrefs for this now, so we should (for now) only be getting this one pref change.

> setTitle(mShowUrl ? tab.getUrl() : tab.getDisplayTitle());
Done. I'll move the formatting from the other bug into setTitle();

> refresh is not lightweight. Can't we just change the title? just call
> setTitle here?
Switched

> I do wonder about the performance implications with using a listener
Everything is background threaded as much as possible now I think.

> "Display Address in Toolbar"
> "Display Title in Toolbar"
I did this, but I don't think I really like it. It feels redudant for the menu to refer to the thing the menu was attached to. Sorta like "Show the Address in that thing you just clicked on there". I'd rather just Display Address and Display Title.
(Assignee)

Updated

5 years ago
Attachment #735280 - Flags: review?(mark.finkle)
Comment on attachment 735280 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>-    public BrowserToolbar(BrowserApp activity) {
>+    public BrowserToolbar(final BrowserApp activity) {

Does this need to be final?

>+        final BrowserToolbar self = this;
>+        (new UiAsyncTask<Void, Void, Void>(ThreadUtils.getBackgroundHandler()) {
>+            @Override
>+            public synchronized Void doInBackground(Void... params) {
>+                SharedPreferences settings = activity.getSharedPreferences(PREFS_NAME, 0);
>+                settings.registerOnSharedPreferenceChangeListener(self);

I think you could use:
settings.registerOnSharedPreferenceChangeListener(BrowserToolbar.this);

and remove:
final BrowserToolbar self = this;

Maybe. Maybe not since it's a thread.

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_show_url "Display Address in Toolbar">
>+<!ENTITY contextmenu_show_title "Display Title in Toolbar">

I'd be fine with dropping the "in Toolbar" from the strings. I just worry a little about users not knowing that this menu will change the text in the URLBar. That's one reason I like "Display" over "Show". We use "Show" to pop things up in front of the user.

Can you make a build and let's get Ian and others to give feedback before landing.

r+, but this is contingent on getting UX to OK as well.
Attachment #735280 - Flags: review?(mark.finkle) → review+
Ian - We need your thoughts on this approach. Wes is going to get a build to try out.
Flags: needinfo?(ibarlow)
Ian, Wes: If this approach seems too "in your face" or confusing, we could try:
* Visible setting?
* Add enough internal support to allow an add-on to "flip the switch". Maybe add a Gecko pref which could be set via about:config _or_ and add-on?
Comment on attachment 735280 [details] [diff] [review]
Patch v2

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

::: mobile/android/base/BrowserToolbar.java
@@ +468,5 @@
>      public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
>          switch(msg) {
>              case TITLE:
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
> +                    setTitle(mShowUrl ? tab.getURL() : tab.getDisplayTitle());

Drive-by: "mShowUrl ? tab.getURL() : tab.getDisplayTitle()" should probably be factored out into a method to avoid the code duplication.
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Ian, Wes: If this approach seems too "in your face" or confusing, we could
> try:
> * Visible setting?
> * Add enough internal support to allow an add-on to "flip the switch". Maybe
> add a Gecko pref which could be set via about:config _or_ and add-on?

I would suggest making the strings sentence case (Display title in address bar)

Wes, can you please post a build for me to try?
Flags: needinfo?(ibarlow)
(In reply to Mark Finkle (:mfinkle) from comment #10)
> Ian, Wes: If this approach seems too "in your face" or confusing, we could
> try:
> * Visible setting?
> * Add enough internal support to allow an add-on to "flip the switch". Maybe
> add a Gecko pref which could be set via about:config _or_ and add-on?

I'm not keen on adding this option to the toolbar context menu because it currently only deals with actions on the current website (share, copy URL, site settings, etc), not the UI.

I'd personally prefer to see this under our "Privacy and Security" settings with the wording suggested by Ian ("Display XXXXX in address bar"). Not sure if XXXXX should use "title" (on by default) or "URL" (off by default) though. URL is maybe too technical.
(In reply to Lucas Rocha (:lucasr) from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #10)
> > Ian, Wes: If this approach seems too "in your face" or confusing, we could
> > try:
> > * Visible setting?
> > * Add enough internal support to allow an add-on to "flip the switch". Maybe
> > add a Gecko pref which could be set via about:config _or_ and add-on?
> 
> I'm not keen on adding this option to the toolbar context menu because it
> currently only deals with actions on the current website (share, copy URL,
> site settings, etc), not the UI.
> 
> I'd personally prefer to see this under our "Privacy and Security" settings
> with the wording suggested by Ian ("Display XXXXX in address bar"). Not sure
> if XXXXX should use "title" (on by default) or "URL" (off by default)
> though. URL is maybe too technical.

I am leaning towards a Setting too. But instead of a checkbox, I think a list would be better, since it shows both states: Title, Address
(Assignee)

Comment 15

5 years ago
Sorry for the delay. Build at:

http://people.mozilla.com/~wjohnston/showurl.apk

it has both the prefernce and the context menu item (to compare, not because I think we should ship both). I like the context menu better, but mostly because I like the instant feedback. It makes it clearer to me what the pref does.
(Assignee)

Comment 16

5 years ago
Also, my builds on this machine seem to crash anytime you load a page with plugin content. :( Just be careful.
(In reply to Wesley Johnston (:wesj) from comment #15)

> it has both the prefernce and the context menu item (to compare, not because
> I think we should ship both). I like the context menu better, but mostly
> because I like the instant feedback. It makes it clearer to me what the pref
> does.

The instant feedback is nice, I agree, but I also don't see this as a feature people are going to be toggling constantly. I see this more as a 'set it and forget it', which would probably belong in Settings as Lucas suggested.

Other than that this seems to work as intended. The strings read a bit strangely though. Maybe we just need to simplify: "Title bar", with the subheads "Show page title" or "Show URL".
(Assignee)

Comment 18

5 years ago
Created attachment 738064 [details] [diff] [review]
Patch v3

Mostly the same. Just uses the pref instead. Updated the strings to match ian's. I'm not sure if we use "Titlebar" or "Title bar", but this uses the former.
Attachment #735020 - Attachment is obsolete: true
Attachment #735280 - Attachment is obsolete: true
Attachment #738064 - Flags: review?(mark.finkle)
(Assignee)

Comment 19

5 years ago
Created attachment 738152 [details] [diff] [review]
Patch v4

Cleaned up the prefs stuff a bit.
Attachment #738064 - Attachment is obsolete: true
Attachment #738064 - Flags: review?(mark.finkle)
Attachment #738152 - Flags: review?(mark.finkle)
Comment on attachment 738152 [details] [diff] [review]
Patch v4

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>-    public boolean onContextItemSelected(MenuItem item) {
>+    public boolean onContextItemSelected(final MenuItem item) {

Does this need to be final?

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+        final BrowserToolbar self = this;
>+        (new UiAsyncTask<Void, Void, Void>(ThreadUtils.getBackgroundHandler()) {
>+            @Override
>+            public synchronized Void doInBackground(Void... params) {
>+                SharedPreferences settings = activity.getSharedPreferences(PREFS_NAME, 0);
>+                settings.registerOnSharedPreferenceChangeListener(self);

I think you could use:
settings.registerOnSharedPreferenceChangeListener(BrowserToolbar.this);

and remove:
final BrowserToolbar self = this;

Maybe. Maybe not since it's a thread.

r- but make these changes and we'll get this landed
Attachment #738152 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 21

5 years ago
Created attachment 742063 [details] [diff] [review]
Patch v2

Updated to address comments.
Attachment #738152 - Attachment is obsolete: true
Attachment #742063 - Flags: review?(mark.finkle)
(Assignee)

Comment 22

5 years ago
Created attachment 742065 [details] [diff] [review]
Patch v2.1

Now without cruft!
Attachment #742063 - Attachment is obsolete: true
Attachment #742063 - Flags: review?(mark.finkle)
Attachment #742065 - Flags: review?(mark.finkle)
Comment on attachment 742065 [details] [diff] [review]
Patch v2.1

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>+    public static final String PREFS_NAME = "BrowserToolbarPrefs";

To be consistent with GeckoApp, let's rename this to "BrowserToolbar"   (no "Prefs" suffix)

r+ with that change
Attachment #742065 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d8817e53bb
https://hg.mozilla.org/mozilla-central/rev/19d8817e53bb
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox23: --- → verified
Flags: in-moztrap?(fennec)
Blocks: 868845

Updated

4 years ago
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → ---

Updated

4 years ago
relnote-firefox: --- → ?
relnote-firefox: ? → 23+
Test Case added in moztrap:
https://moztrap.mozilla.org/manage/case/8075/
https://moztrap.mozilla.org/manage/case/8076/
Flags: in-moztrap?(fennec) → in-moztrap+
This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/mobile/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.
You need to log in before you can comment on or make changes to this bug.