944925 - Add an option to switch dynamic toolbar mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Add an option to switch dynamic toolbar mode.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v1

Comment 2

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

This kind of change needs approval from the UX team as well. I know we were talking about changing the default for tablets. Adding Ian to get some feedback on how we'd like to proceed, if at all.

Comment 3

[Aaron Train [:aaronmt]]

Dupe of bug 888690?

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Aaron Train [:aaronmt] from comment #3)
> Dupe of bug 888690?

I don't intend to change the default value of `browser.chrome.dynamictoolbar`. I think there'is no problem that we ship `browser.chrome.dynamictoolbar` as true.

Comment 5

[:Margaret Leibovic]

I agree we should get some UX input before exposing this as a user-visible pref. At the very least, it would be good to get input on what strings to use.

Comment 6

[Ian Barlow (:ibarlow)]

I think adding a line in Settings to toggle the scrolling toolbar on and off sounds fine. We should default the pref "on" for phones, and "off" for tablets, and probably place it under "Customize".



< Customize
-------------------------------------
  Search Settings
-------------------------------------
  Hide title bar when scrolling   [x]
-------------------------------------
  Tabs
  Always restore
-------------------------------------
  Download updates automatically
  Always
-------------------------------------
  Import from Android
-------------------------------------

Comment 7

[:Margaret Leibovic]

Comment on attachment 8340677 [details] [diff] [review]
patch v1

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

We should update this patch to match what ibarlow specified in comment 6.

To address that comment, we should make this into a CheckboxPreference, and then we should be able to just read the value as a boolean.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Ian Barlow (:ibarlow) from comment #6)
> I think adding a line in Settings to toggle the scrolling toolbar on and off
> sounds fine. We should default the pref "on" for phones, and "off" for
> tablets, and probably place it under "Customize".

The preference for titlebar has already been in "Display" category. Do we place this to "Customaize" really?

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part 1 v1: Change the order of "Customize" pane in Fennec option.

Change the order to follow Ian's comment 6.

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Comment on attachment 8341975 [details] [diff] [review]
part 1 v1:  Change the order of "Customize" pane in Fennec option.

># HG changeset patch
># Parent 84a5a5800bd3b45ec08b1cc77b3f14822b2a33c2
># User Tetsuharu OHZEKI <[e-mail]>
>Bug 944925 - part 1: Change the order of "Customize" pane in Fennec option. r=?
>
>diff --git a/mobile/android/base/resources/xml/preferences_customize.xml b/mobile/android/base/resources/xml/preferences_customize.xml
>--- a/mobile/android/base/resources/xml/preferences_customize.xml
>+++ b/mobile/android/base/resources/xml/preferences_customize.xml
>@@ -11,33 +11,33 @@
>                 android:targetPackage="@string/android_package_name"
>                 android:targetClass="org.mozilla.gecko.preferences.GeckoPreferences" >
>             <extra
>                 android:name="resource"
>                 android:value="preferences_search" />
>         </intent>
>     </PreferenceScreen>
> 
>-    <org.mozilla.gecko.preferences.AndroidImportPreference
>-                  android:key="android.not_a_preference.import_android"
>-                  gecko:entries="@array/pref_import_android_entries"
>-                  gecko:entryKeys="@array/pref_import_android_keys"
>-                  gecko:initialValues="@array/pref_import_android_values"
>-                  android:title="@string/pref_import_android"
>-                  android:positiveButtonText="@string/bookmarkhistory_button_import"
>-                  android:negativeButtonText="@string/button_cancel"
>-                  android:persistent="false" />
>-
>     <ListPreference android:key="android.not_a_preference.restoreSession3"
>                     android:title="@string/pref_restore"
>                     android:defaultValue="quit"
>                     android:entries="@array/pref_restore_entries"
>                     android:entryValues="@array/pref_restore_values"
>                     android:persistent="true" />
> 
>    <ListPreference android:key="app.update.autodownload"
>                    android:title="@string/pref_update_autodownload"
>                    android:entries="@array/pref_update_autodownload_entries"
>                    android:entryValues="@array/pref_update_autodownload_values"
>                    android:persistent="false" />
> 
>+    <org.mozilla.gecko.preferences.AndroidImportPreference
>+                  android:key="android.not_a_preference.import_android"
>+                  gecko:entries="@array/pref_import_android_entries"
>+                  gecko:entryKeys="@array/pref_import_android_keys"
>+                  gecko:initialValues="@array/pref_import_android_values"
>+                  android:title="@string/pref_import_android"
>+                  android:positiveButtonText="@string/bookmarkhistory_button_import"
>+                  android:negativeButtonText="@string/button_cancel"
>+                  android:persistent="false" />
>+
> </PreferenceScreen>
>

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part 2 v2: Add an option to switch dynamic toolbar mode

Rebased on Ian's comment 6

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

try: https://tbpl.mozilla.org/?tree=Try&rev=8d8ca4c13291

Comment 13

[:Margaret Leibovic]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #8)
> (In reply to Ian Barlow (:ibarlow) from comment #6)
> > I think adding a line in Settings to toggle the scrolling toolbar on and off
> > sounds fine. We should default the pref "on" for phones, and "off" for
> > tablets, and probably place it under "Customize".
> 
> The preference for titlebar has already been in "Display" category. Do we
> place this to "Customaize" really?

This is a good point... I agree we should probably put these two title bar prefs in the same place. Ian, what do you think?

Also, did you mean to switch the placement of the "Import from Android" preference? If we do decide to put this dynamic toolbar pref in the "Display" section, would you still want to change the ordering in "Customize"?

Comment 14

[:Margaret Leibovic]

Comment on attachment 8341975 [details] [diff] [review]
part 1 v1:  Change the order of "Customize" pane in Fennec option.

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

Let's wait to hear back from Ian before landing this (sorry I didn't notice that inconsistency in his comment earlier).

Comment 15

[:Margaret Leibovic]

Comment on attachment 8341977 [details] [diff] [review]
part 2 v2: Add an option to switch dynamic toolbar mode

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

Looks good. Once again, let's just hear back from Ian to see if we should move this into the "Display" category, since I think that's a good idea.

::: mobile/android/base/resources/xml/preferences_customize.xml
@@ +13,5 @@
>              <extra
>                  android:name="resource"
>                  android:value="preferences_search" />
>          </intent>
> +      </PreferenceScreen>

Oops, I don't think you meant to change the indentation here.

Comment 16

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Tetsuharu OHZEKI [UTC+9] from comment #8)
> > (In reply to Ian Barlow (:ibarlow) from comment #6)
> > > I think adding a line in Settings to toggle the scrolling toolbar on and off
> > > sounds fine. We should default the pref "on" for phones, and "off" for
> > > tablets, and probably place it under "Customize".
> > 
> > The preference for titlebar has already been in "Display" category. Do we
> > place this to "Customaize" really?
> 
> This is a good point... I agree we should probably put these two title bar
> prefs in the same place. Ian, what do you think?
> 

Yeah, that's a good point. It seemed more like a "customization" thing, but those two prefs should probably live together. Kind of leads into a separate question of whether a section called "customization" even makes sense at all, since everything you do in Settings is technically some form of customization... That's a discussion for another bug, though :)

Let's move this pref to the "display" section, under the other title bar pref.

Comment 17

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Ian Barlow (:ibarlow) from comment #16)
> Let's move this pref to the "display" section, under the other title bar
> pref.

OK. I'll change to it in the next patch :)

BTW, How about do you think about Margaret's this comment #14?

Comment 18

[Ian Barlow (:ibarlow)]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #17)
> (In reply to Ian Barlow (:ibarlow) from comment #16)
> > Let's move this pref to the "display" section, under the other title bar
> > pref.
> 
> OK. I'll change to it in the next patch :)
> 
> BTW, How about do you think about Margaret's this comment #14?

Oops, I missed that point. You can leave "import from android" where it is for now, thanks.

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3

patch v3!

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

Comment 20

[:Margaret Leibovic]

Comment on attachment 8343032 [details] [diff] [review]
patch v3

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

Excellent!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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