Closed Bug 944925 Opened 6 years ago Closed 6 years ago

Add an option to switch dynamic toolbar mode

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
relnote-firefox --- 28+

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

(Keywords: feature)

Attachments

(1 file, 3 obsolete files)

Add an option to switch dynamic toolbar mode.
Component: Awesomescreen → General
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8340677 - Flags: review?(margaret.leibovic)
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.
Flags: needinfo?(ibarlow)
(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.
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.
Assignee: nobody → saneyuki.s.snyk
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
-------------------------------------
Assignee: saneyuki.s.snyk → nobody
Flags: needinfo?(ibarlow)
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.
Attachment #8340677 - Flags: review?(margaret.leibovic) → review-
(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?
Change the order to follow Ian's comment 6.
Attachment #8340677 - Attachment is obsolete: true
Attachment #8341975 - Flags: review?(margaret.leibovic)
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 <saneyuki.s.snyk@gmail.com>
>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>
>
Attachment #8341975 - Attachment is patch: true
Rebased on Ian's comment 6
Attachment #8341977 - Flags: review?(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"?
Flags: needinfo?(ibarlow)
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).
Attachment #8341975 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8341977 - Flags: review?(margaret.leibovic) → review+
(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.
Flags: needinfo?(ibarlow)
(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?
(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.
Attached patch patch v3Splinter Review
patch v3!

try: https://tbpl.mozilla.org/?tree=Try&rev=d1164df46eda
Attachment #8341975 - Attachment is obsolete: true
Attachment #8341977 - Attachment is obsolete: true
Attachment #8343032 - Flags: review?(margaret.leibovic)
Comment on attachment 8343032 [details] [diff] [review]
patch v3

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

Excellent!
Attachment #8343032 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/integration/fx-team/rev/d7afefc2546a
Assignee: nobody → saneyuki.s.snyk
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
blocking-fx: --- → ?
Keywords: feature
https://hg.mozilla.org/mozilla-central/rev/d7afefc2546a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
blocking-fx: ? → ---
relnote-firefox: --- → ?
Duplicate of this bug: 949328
Depends on: 961375
Duplicate of this bug: 906674
You need to log in before you can comment on or make changes to this bug.