Closed Bug 1009376 Opened 10 years ago Closed 10 years ago

Add android:summary for "Scroll title bar"

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 34

People

(Reporter: rnewman, Assigned: aymen, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(3 files, 2 obsolete files)

Ain't nobody gonna know what "Scroll title bar" means.
---
Screens of phones are longer than necessary nowadays. Thus there is no need to hide header tab when scrolling up. Give users option to disable as we need to compare different tabs on certain part of the Web page but the tabs selection is always missing and require you to readjust by scrolling.
https://input.mozilla.org/en-US/dashboard/response/4357330
---
Blocks: 965377
Can we make this "Auto-hide title bar"? (Or Autohide.) I don't think we need a summary for it.
Flags: needinfo?(ibarlow)
Assignee: nobody → eedens
(In reply to Chenxia Liu [:liuche] from comment #2)
> Can we make this "Auto-hide title bar"? (Or Autohide.) I don't think we need
> a summary for it.

I think that'd need to be "Auto-hide title bar when scrolling page", which might not fit -- otherwise we're just ditching the other half of the concept!

I think perhaps the Androidy thing to do is:


   Auto-hide title bar          [ ]
   Title bar always visible.


   Auto-hide title bar          [/]
   Title bar hides until you scroll up.


or somesuch.
I think we should avoid using the second line as a status indicator. From my reading of the Android style guide, the second line is a description for checkboxes, but status for everything else. (Reference: "...unless it's a checkbox setting" on http://developer.android.com/design/patterns/settings.html).

How about:

  Auto-hide title bar   [ ]
  Hide title bar when scrolling down.
Attached image DisplayPref_v1.png (obsolete) —
Pocket uses:

  Auto fullscreen                         [ ]
  Hide toolbars while reading/scrolling
  down. Reveal toolbars while scrolling
  up

Doesn't seem great to me, but does at least include how you get the title bar to come back.
Attached image DisplayPref_v2.png
I like the Pocket example -- in fact, using 'fullscreen' feels more descriptive. Here's a second shot.
Attachment #8422158 - Attachment is obsolete: true
(In reply to Eric Edens [:eedens] from comment #7)
> Created attachment 8422164 [details]
> DisplayPref_v2.png
> 
> I like the Pocket example -- in fact, using 'fullscreen' feels more
> descriptive. Here's a second shot.

This might be a little verbose for translating to other languages. Given the information in this bug, let's see what Ian and UX come up with.

Waiting for feedback from UX.
Here's the patch for updating the title and description for this menu setting. We can adjust the verbage as needed from UX
Comment on attachment 8422688 [details] [diff] [review]
WIP - Change setting name; add setting description

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

Just a quick feedback drive-by.

Nice first patch!

One other thing that you should make sure to do in this patch is update the robocop test for Settings. This will be in mobile/android/base/tests/testSettingsMenuItems.java. Be sure to add the summary to the strings that are checked there. (If the comments aren't clear, update them so they make sense :) For funsies, you could even try to get robocop tests running locally (which can annoying, but saves a lot of time waiting for try runs): https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop .

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +276,5 @@
>  <!ENTITY pref_titlebar_mode_url "Show page address">
>  
>  <!-- Localization note (pref_scroll_title_bar): Label for setting that controls
>       whether or not the dynamic toolbar is enabled. -->
> +<!ENTITY pref_scroll_title_bar "Auto fullscreen">

One thing that's tricky about localizations is handling changes to old strings. If we change an existing string entity, you need to rename the title so that localizers know that this is a new string to be localized.

So, in this case, you'll want to also rename this to pref_scroll_title_bar2.

(When adding a new string resource, like the summary, you don't need to do anything.)

::: mobile/android/base/resources/xml/preferences_display.xml
@@ +22,5 @@
>                      android:persistent="false" />
>  
>      <CheckBoxPreference android:key="browser.chrome.dynamictoolbar"
> +                        android:title="@string/pref_scroll_title_bar"
> +                        android:summary="@string/pref_scroll_title_bar_summary" />

Good. One thing to note - if you change preferences in the future, keep in mind that for non-leaf preference screens (e.g., screens that can open other preference screens), our prefs are also mirrored to tablet xml files and xml files in /xml-v11, so you may need to change them there.
(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Eric Edens [:eedens] from comment #7)
> > Created attachment 8422164 [details]
> > DisplayPref_v2.png
> > 
> > I like the Pocket example -- in fact, using 'fullscreen' feels more
> > descriptive. Here's a second shot.
> 
> This might be a little verbose for translating to other languages. Given the
> information in this bug, let's see what Ian and UX come up with.
> 
> Waiting for feedback from UX.




How about something like:


-------------------------------------------------------
Full screen browsing  [x]
Hide the Firefox title bar when scrolling down a page
-------------------------------------------------------
Flags: needinfo?(ibarlow)
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [lang=java][good first bug]
I'll have a go at this - seems like it's been dropped.
(In reply to Ian Barlow (:ibarlow) from comment #12)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > (In reply to Eric Edens [:eedens] from comment #7)
> > > Created attachment 8422164 [details]
> > > DisplayPref_v2.png
> > > 
> > > I like the Pocket example -- in fact, using 'fullscreen' feels more
> > > descriptive. Here's a second shot.
> > 
> > This might be a little verbose for translating to other languages. Given the
> > information in this bug, let's see what Ian and UX come up with.
> > 
> > Waiting for feedback from UX.
> 
> 
> 
> 
> How about something like:
> 
> 
> -------------------------------------------------------
> Full screen browsing  [x]
> Hide the Firefox title bar when scrolling down a page
> -------------------------------------------------------

What do you think of this?:

---------------------------------------------------------
Full screen browsing [ ]
Always show the Firefox title bar
---------------------------------------------------------
Full screen browsing [X]
Hide the Firefox title bar when scrolling down a page
---------------------------------------------------------

I guess that's more in-line with android's settings itself, but I'm not sure.
Flags: needinfo?(ibarlow)
(In reply to Aymen Qader from comment #14)

> What do you think of this?:
> 
> ---------------------------------------------------------
> Full screen browsing [ ]
> Always show the Firefox title bar
> ---------------------------------------------------------
> Full screen browsing [X]
> Hide the Firefox title bar when scrolling down a page
> ---------------------------------------------------------
> 
> I guess that's more in-line with android's settings itself, but I'm not sure.


I actually don't think the summary should change state like when it's checked/unchecked, as it seems make the unchecked state more confusing.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #15)
> (In reply to Aymen Qader from comment #14)
> 
> > What do you think of this?:
> > 
> > ---------------------------------------------------------
> > Full screen browsing [ ]
> > Always show the Firefox title bar
> > ---------------------------------------------------------
> > Full screen browsing [X]
> > Hide the Firefox title bar when scrolling down a page
> > ---------------------------------------------------------
> > 
> > I guess that's more in-line with android's settings itself, but I'm not sure.
> 
> 
> I actually don't think the summary should change state like when it's
> checked/unchecked, as it seems make the unchecked state more confusing.

Alright, I'll get to work on updating it with a static summary.
Assignee: eric.edens → aymen
Attached patch 1009376_1.patch (obsolete) — Splinter Review
Changed setting string according to :ibarlow's instructions.
Attachment #8461760 - Flags: review?(rnewman)
Comment on attachment 8461760 [details] [diff] [review]
1009376_1.patch

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

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +307,2 @@
>       whether or not the dynamic toolbar is enabled. -->
> +<!ENTITY pref_scroll_title_bar2 "Full screen browsing">

Should be "Full-screen browsing".

@@ +307,3 @@
>       whether or not the dynamic toolbar is enabled. -->
> +<!ENTITY pref_scroll_title_bar2 "Full screen browsing">
> +<!ENTITY pref_scroll_title_bar_summary "Hide the Firefox title bar when scrolling down a page">

This should use &brandShortName; instead of hard-coding "Firefox".

::: mobile/android/base/strings.xml.in
@@ +296,5 @@
>    <string name="pref_titlebar_mode">&pref_titlebar_mode;</string>
>    <string name="pref_titlebar_mode_title">&pref_titlebar_mode_title;</string>
>    <string name="pref_titlebar_mode_url">&pref_titlebar_mode_url;</string>
>  
> +  <string name="pref_scroll_title_bar">&pref_scroll_title_bar2;</string>

Let's rename the string ("2") to match the entity, just to avoid confusion.

::: mobile/android/base/tests/StringHelper.java
@@ +110,5 @@
>  
>      // Display
>      public static final String TEXT_SIZE_LABEL = "Text size";
>      public static final String TITLE_BAR_LABEL = "Title bar";
> +    public static final String SCROLL_TITLE_BAR_LABEL="Full screen browsing";

Should be "Full-screen browsing".

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +48,5 @@
>      String[] PATH_DISPLAY = { StringHelper.DISPLAY_SECTION_LABEL };
>      String[][] OPTIONS_DISPLAY = {
>          { StringHelper.TEXT_SIZE_LABEL },
>          { StringHelper.TITLE_BAR_LABEL, "Show page title", "Show page title", "Show page address" },
> +	{ StringHelper.SCROLL_TITLE_BAR_LABEL, "Hide the Firefox title bar when scrolling down a page" }, 

Nit: trailing whitespace, and you'll need to adapt to the product name in this string, too. See `BRAND_NAME` earlier in the file for how to do that.
Attachment #8461760 - Flags: review?(rnewman) → feedback+
Attached patch 1009376_2.patchSplinter Review
Sorry for late upload; was busy.
Attachment #8461760 - Attachment is obsolete: true
Attachment #8462028 - Flags: review?(rnewman)
Comment on attachment 8462028 [details] [diff] [review]
1009376_2.patch

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

Looks fantastic. I'll queue this up to land with the correct commit message. Thanks!
Attachment #8462028 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7912f1ea1f9a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Going to Settings -> Display, the third option is:
-------------------------------------------------------
Full-screen browsing  [x]
Hide the Firefox title bar when scrolling down a page
-------------------------------------------------------
Verified as fixed on:
Builds:37 Nightly and 36 Aurora(2014-12-09), 35 beta 1 and 34 Release
Device: Samsung Galaxy Tab (Android 4.2)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.