Closed
Bug 1247297
Opened 9 years ago
Closed 9 years ago
[Tablet] Tablets are missing dynamic toolbar pref in settings UI
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox44 unaffected, firefox45 verified, firefox46 verified, firefox47 verified, fennec45+)
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)
1.52 KB,
patch
|
sebastian
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Sebastian, could you help write a quick patch for this?
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
Comment 2•9 years ago
|
||
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
Comment 4•9 years ago
|
||
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)
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.
Updated•9 years ago
|
Assignee: nobody → mihai.g.pop
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment 6•9 years ago
|
||
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 8•9 years ago
|
||
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
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
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?
Comment 11•9 years ago
|
||
Keywords: checkin-needed
![]() |
||
Comment 12•9 years ago
|
||
This patch landed 13 hours ago with the wrong number - https://hg.mozilla.org/mozilla-central/rev/8dcebf7d880c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•9 years ago
|
Keywords: regression
Comment 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
bugherder uplift |
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Verified as fixed Firefox 45 Beta 6, and on latest Aurora and Nightly
Status: RESOLVED → VERIFIED
Updated•5 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
•