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)
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)
95.11 KB,
image/png
|
Details | |
3.62 KB,
patch
|
Details | Diff | Splinter Review | |
7.20 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
Ain't nobody gonna know what "Scroll title bar" means.
Reporter | ||
Comment 1•10 years ago
|
||
--- 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 ---
Comment 2•10 years ago
|
||
Can we make this "Auto-hide title bar"? (Or Autohide.) I don't think we need a summary for it.
Flags: needinfo?(ibarlow)
Updated•10 years ago
|
Assignee: nobody → eedens
Reporter | ||
Comment 3•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
Reporter | ||
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
I like the Pocket example -- in fact, using 'fullscreen' feels more descriptive. Here's a second shot.
Attachment #8422158 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
Here's the patch for updating the title and description for this menu setting. We can adjust the verbage as needed from UX
Comment 10•10 years ago
|
||
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.
Comment 12•10 years ago
|
||
(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)
Updated•10 years ago
|
Mentor: rnewman
Whiteboard: [mentor=rnewman][lang=java][good first bug] → [lang=java][good first bug]
Assignee | ||
Comment 13•10 years ago
|
||
I'll have a go at this - seems like it's been dropped.
Assignee | ||
Comment 14•10 years ago
|
||
(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)
Comment 15•10 years ago
|
||
(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)
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Updated•10 years ago
|
Assignee: eric.edens → aymen
Assignee | ||
Comment 17•10 years ago
|
||
Changed setting string according to :ibarlow's instructions.
Attachment #8461760 -
Flags: review?(rnewman)
Reporter | ||
Comment 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
Sorry for late upload; was busy.
Attachment #8461760 -
Attachment is obsolete: true
Attachment #8462028 -
Flags: review?(rnewman)
Reporter | ||
Comment 20•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7912f1ea1f9a
https://hg.mozilla.org/mozilla-central/rev/7912f1ea1f9a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 23•9 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•