Closed Bug 1048575 Opened 6 years ago Closed 6 years ago

Disable dynamic toolbar when in new tablet UI

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file, 1 obsolete file)

With the new tab strip, we'll keep the toolbar always visible in v1.
Comment on attachment 8468509 [details] [diff] [review]
Disable dynamic toolbar when in new tablet UI (r=mcomella)

For v1, we'll simply disable dynamic toolbar behaviour on the new tablet UI.
Attachment #8468509 - Flags: review?(michael.l.comella)
Comment on attachment 8468553 [details] [diff] [review]
Disable dynamic toolbar when in new tablet UI (r=mcomella)

Uses the final API for new tablet UI checks.
Attachment #8468553 - Flags: review?(michael.l.comella)
Attachment #8468509 - Attachment is obsolete: true
Attachment #8468509 - Flags: review?(michael.l.comella)
Comment on attachment 8468553 [details] [diff] [review]
Disable dynamic toolbar when in new tablet UI (r=mcomella)

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

::: mobile/android/base/DynamicToolbar.java
@@ +140,5 @@
>      private class PrefHandler extends PrefHandlerBase {
> +        private final Context context;
> +
> +        public PrefHandler(Context context) {
> +            this.context = context.getApplicationContext();

I'd either call this `applicationContext`, or reserve the `.getApplicationContext()` call when it's actually used below. I'd prefer the latter, so we're less likely to accidentally leak the application context.

@@ +156,5 @@
>                  @Override
>                  public void run() {
> +                    // If either accessibility or new tablet UI are enabled,
> +                    // the dynamic toolbar is forced to be off.
> +                    if (!accessibilityEnabled && !NewTabletUI.isEnabled(context)) {

I don't understand why we short-circuit running the onEnabledChanged handler under these conditions - is it because we default to having the dynamic toolbar on? Why not assert these values *before* posting to the UI thread? Shouldn't this be taken into account with prefEnabled above?

Additionally, I think we should add some handling to the `isEnabled` method, as it seems to be duplicated here (...but given my reservations above, why is that? Shouldn't be just call `isEnabled()` to get the pref value here instead of duplicating?).
Attachment #8468553 - Flags: review?(michael.l.comella) → feedback+
Stepping back a bit, I think we should keep the dynamic toolbar behaviour in the new tablet UI where both the toolbar and tabstrip slide in and out as you scroll web content. Keeping such a big chrome area always around is pretty bad design, especially on small tablets. We can refine the behaviour with fancier stuff if we want too but this is good enough for v1. I'll bring this up in our next tablet meeting.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Would be good if there would be an option to stick only tab bar, while keeping adressbar dynamic. Would offer better usability on large tablets and especially on 4:3 tablets.
You need to log in before you can comment on or make changes to this bug.