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

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[u421692]

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

Comment 1

[:Margaret Leibovic]

Sebastian, could you help write a quick patch for this?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Are you sure about firefox44 being unaffected? The toolbar is scrolling away on my Nexus 9 with Firefox 44.0.

Comment 3

[u421692]

(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

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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.

Comment 5

[u421692]

Attachment: Bug1247297.patch

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.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 7

[u421692]

Attachment: Bug1247297_v2.patch

(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.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8718321 [details] [diff] [review]
Bug1247297_v2.patch

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

LGTM

Comment 9

[:Margaret Leibovic]

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.

Comment 10

[u421692]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Guilherme Lima]

This patch landed 13 hours ago with the wrong number - https://hg.mozilla.org/mozilla-central/rev/8dcebf7d880c

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8718321 [details] [diff] [review]
Bug1247297_v2.patch

Improve the tablet support, taking it.
Should be in 45 beta 6.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ae4ff32b87e

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9eca5074cb77

Comment 16

[u421692]

Verified as fixed Firefox 45 Beta 6, and on latest Aurora and Nightly