Closed Bug 1247297 Opened 4 years ago Closed 4 years ago

[Tablet] Tablets are missing dynamic toolbar pref in settings UI

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox44 --- unaffected
firefox45 --- verified
firefox46 --- verified
firefox47 --- verified
fennec 45+ ---

People

(Reporter: u421692, Assigned: u421692)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=1224569#c1 
Based on the discussion with margaret and antlam on irc, we should disable "Full-screen browsing" on tablets, since the setting was removed in bug 1216257
Sebastian, could you help write a quick patch for this?
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
Are you sure about firefox44 being unaffected? The toolbar is scrolling away on my Nexus 9 with Firefox 44.0.
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Are you sure about firefox44 being unaffected? The toolbar is scrolling away
> on my Nexus 9 with Firefox 44.0.

That's right, but in Firefox 44, "Full-screen browsing" settings is still present in Settings->Display.
The setting was removed in Bug 1216257, which made it in Firefox 45
Actually, I'm testing this on my N9 right now, Fx 44, and the Setting is there (in Display) AND it works. Turning it on and off _does_ affect the toolbar.

Sorry, I may have spoke too soon. I think I made a mistake when I said that we didn't want this option on tablets - we do.

Looking at the behaviour of other browsers on tablets as well, I think this is the expected behaviour here.

So, I think we should maintain this functionality and Setting for Tablets.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
Attached patch Bug1247297.patch (obsolete) — Splinter Review
This was removed in bug 1218557, and I've just readded the code.
It's my first bug, so it might need retouches.
I've tested this on a local build on Nexus 9 and Alcatel 8008D, and it works fine, "Full-screen browsing" is displayed in "Settings"->"General", and works as expected.
Assignee: nobody → mihai.g.pop
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment on attachment 8718259 [details] [diff] [review]
Bug1247297.patch

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

The patch is looking good. Just add a commit message and consider moving the pref up (see below).

::: mobile/android/base/resources/xml-v11/preferences_general_tablet.xml
@@ +36,5 @@
>                          android:defaultValue="false" />
>  
> +    <CheckBoxPreference android:key="browser.chrome.dynamictoolbar"
> +                        android:title="@string/pref_scroll_title_bar2"
> +                        android:summary="@string/pref_scroll_title_bar_summary" />

Should we move this up so that it's between "Language" and "Tab queue" like on phones?
Attachment #8718259 - Flags: feedback+
(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8718259 [details] [diff] [review]
> Bug1247297.patch
> 
> Review of attachment 8718259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch is looking good. Just add a commit message and consider moving the
> pref up (see below).
> 
> ::: mobile/android/base/resources/xml-v11/preferences_general_tablet.xml
> @@ +36,5 @@
> >                          android:defaultValue="false" />
> >  
> > +    <CheckBoxPreference android:key="browser.chrome.dynamictoolbar"
> > +                        android:title="@string/pref_scroll_title_bar2"
> > +                        android:summary="@string/pref_scroll_title_bar_summary" />
> 
> Should we move this up so that it's between "Language" and "Tab queue" like
> on phones?

I've made the requested changes, thanks for the review.
Attachment #8718259 - Attachment is obsolete: true
Attachment #8718321 - Flags: review?(s.kaspari)
Comment on attachment 8718321 [details] [diff] [review]
Bug1247297_v2.patch

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

LGTM
Attachment #8718321 - Flags: review?(s.kaspari) → review+
Keywords: checkin-needed
Mihai, can you also request uplift to aurora and beta? You can do that by flipping the flags in the details page of the patch attachment.
tracking-fennec: ? → 45+
Flags: needinfo?(margaret.leibovic) → needinfo?(mihai.g.pop)
Summary: [Tablet] The toolbar scrolls off screen on tablets → [Tablet] Tablets are missing dynamic toolbar pref in settings UI
Comment on attachment 8718321 [details] [diff] [review]
Bug1247297_v2.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: "Full-screen browsing" feature not available; users can not change the way the toolbar behaves
[Describe test coverage new/current, TreeHerder]: Tested on a local build
[Risks and why]: Low. Just re-added removed code
[String/UUID change made/needed]: none
Flags: needinfo?(mihai.g.pop)
Attachment #8718321 - Flags: approval-mozilla-beta?
Attachment #8718321 - Flags: approval-mozilla-aurora?
This patch landed 13 hours ago with the wrong number - https://hg.mozilla.org/mozilla-central/rev/8dcebf7d880c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8718321 [details] [diff] [review]
Bug1247297_v2.patch

Improve the tablet support, taking it.
Should be in 45 beta 6.
Attachment #8718321 - Flags: approval-mozilla-beta?
Attachment #8718321 - Flags: approval-mozilla-beta+
Attachment #8718321 - Flags: approval-mozilla-aurora?
Attachment #8718321 - Flags: approval-mozilla-aurora+
Verified as fixed Firefox 45 Beta 6, and on latest Aurora and Nightly
You need to log in before you can comment on or make changes to this bug.