1091587 - Improve Private tabs's empty view layout on new tablet UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Lucas Rocha (:lucasr)]

Looks clunky right now (see screenshot).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Current: 7" tab portrait

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Current: 7" tab landscape

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Dynamically layout private tabs panel on 7" tablet

This is a little awful but since you can't specify styles dynamically, seems better than copy-pastaing the layout. Still needs 10" tab values.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch: 7" tab portrait

What say you, antlam?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch: 7" tab landscape

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8521090 [details] [diff] [review]
Dynamically layout private tabs panel on 7" tablet

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

I assume this is necessary because we don't re-inflate the tabs panel container on device rotation? This looks ok as a temporary solution.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> I assume this is necessary because we don't re-inflate the tabs panel
> container on device rotation? This looks ok as a temporary solution.

What do you mean by this? The dynamic styling?

The tabs panel is re-constructed on device rotation (as opposed to just re-inflated).

Comment 8

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

Comment on attachment 8521093 [details]
Patch: 7" tab portrait

I think as a temporary solution, this is fine. But I should add some sort of image there too now that we have all that space. That way, we could be more consistent with the "empty" state of our panels UI too.

Could we vertically center this now that we use the full screen or?

Comment 9

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

Comment on attachment 8521094 [details]
Patch: 7" tab landscape

Maybe we could use the same width from the portrait mode to constrain this content a bit.

As with the vertical screen shot, could we center this?

I can come up with specs if you need. :)

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #9)
> I can come up with specs if you need. :)

That would be very helpful. :) Can you do some for 10" tab too (if it's not the same)?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #8)
> Could we vertically center this now that we use the full screen or?

We should be able to do that.

The specs don't need to be visual if that's more difficult - I just need values.

Comment 12

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

(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Anthony Lam (:antlam) from comment #9)
> > I can come up with specs if you need. :)
> 
> That would be very helpful. :) Can you do some for 10" tab too (if it's not
> the same)?

I think we constrain max width for the content at 300 dp in the Sync Panels (from bug 1014994), let's try that!

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Dynamically layout private tabs panel on 7" tablet

Centering issues, wip.

Comment 14

[Lucas Rocha (:lucasr)]

As discussed on IRC, this is likely happening because we're ignoring the toolbar size when calculating the height of the tabs panel's content area.

Comment 15

[Lucas Rocha (:lucasr)]

For reference: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/TabsPanel.java#246

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> For reference:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabs/
> TabsPanel.java#246

I believe that DisplayMetrics.heightPixels, which we use to determine the size of the view, also includes the notification bar as well as the Android soft system buttons (e.g. back, home, recent apps).

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

According to the docs [1]:

The application display area specifies the part of the display that may contain an application window, excluding the system decorations. The application display area may be smaller than the real display area because the system subtracts the space needed for decor elements such as the status bar. Use the following methods to query the application display area: getSize(Point), getRectSize(Rect) and getMetrics(DisplayMetrics).

---

So DisplayMetrics.heightPixels - actionBarHeight should give the tab container height, however making the correction still does not fully fix the view.

Martyn, you mentioned in the tablet meeting that there was another issue you found with regards to this offset while investigating bug 1100317 - what is that? Does that apply here?

[1]: https://developer.android.com/reference/android/view/Display.html

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Correct the height of the tabs container on new tablet

My fix, to make sure I'm not doing something silly.

screenHeight is 1824 pixels here where 1920 pixels is height of the physical
display. ~100 pixels is large enough that I imagine it'd include both the soft
system buttons and the system status bar.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Test: private tabs panel at bottom

This cooky patch is what I'm using to test whether or not the container is the right size by ensuring the private tabs panel, with layout_gravity=bottom, is fully shown at the bottom - perhaps there's an error here as well?

Comment 20

[Martyn Haigh (:mhaigh)]

The issue I mentioned was Bug 1100317 - it seems the fix you mention in comment 17 (DisplayMetrics.heightPixels - actionBarHeight) was enough to fix my issue - at least I'm getting the expected results with a similar patch

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

After pulling the latest fx-team, including the patch from bug 1100317, I can confirm Martyn's issue is fixed, but mine is not - there must be something else going on in the layout.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (1/2) Full height

Actually, seems we're a few pixels short - see the next image.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (2/2) Limited height

There's still padding missing at the bottom of the container.

I suppose this isn't actually worth all the time I've been spending on it, considering all we want to do is center the content - I'm sure I can eyeball the offset (actionBarHeight - eyeballedHeight).

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #23)
> There's still padding missing at the bottom of the container.

Filed bug 1104133.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch: 7" tab portrait (v2)

300dp width. Let me know if the vertical centering looks off - I had to offset the centering due to bug 1104133.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch: 7" tab landscape (v2)

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Dynamically layout private tabs panel on new tablet

The patch, if the antlam approves.

Comment 28

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

Comment on attachment 8527788 [details]
Patch: 7" tab portrait (v2)

Nicely done!

Comment 29

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

Comment on attachment 8527789 [details]
Patch: 7" tab landscape (v2)

I assume this Feedback was for me :) Thanks Michael! looks great.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8527806 [details] [diff] [review]
Dynamically layout private tabs panel on new tablet

Despite the description, this should also apply to 10" tablets.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 8527806 [details] [diff] [review]
Dynamically layout private tabs panel on new tablet

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

Yep.

::: mobile/android/base/resources/layout/tabs_panel_default.xml
@@ +5,5 @@
>  
>  <merge xmlns:android="http://schemas.android.com/apk/res/android"
>         xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
> +    <!-- This value is used in TabsPanel.getTabsLayoutContainerHeight

What value?

::: mobile/android/base/tabs/PrivateTabsPanel.java
@@ +116,5 @@
> +
> +        // The tabs panel header is not taken into account
> +        // when centering, so add padding to compensate.
> +        lp.gravity = Gravity.CENTER;
> +        emptyTabsFrame.setPadding(0, 0, 0, emptyTabsFrameVerticalOffset);

Is this still necessary even with the patch from bug 1100317?

@@ +120,5 @@
> +        emptyTabsFrame.setPadding(0, 0, 0, emptyTabsFrameVerticalOffset);
> +
> +        emptyTabsHeader.setVisibility(View.VISIBLE);
> +
> +        requestLayout();

This is not strictly necessary as setPadding() and setVisibility() will trigger requestLayout() under the hood.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Dynamically layout private tabs panel on new tablet

(In reply to Lucas Rocha (:lucasr) from comment #31)
> What value?

Fixed.

> ::: mobile/android/base/tabs/PrivateTabsPanel.java
> > +        emptyTabsFrame.setPadding(0, 0, 0, emptyTabsFrameVerticalOffset);
> 
> Is this still necessary even with the patch from bug 1100317?

Yes, we're trying to center the content on the screen, not in the View, so
we offset for the parent container.

Adjusted the comment to make this more clear.

> @@ +120,5 @@
> > +        requestLayout();
> 
> This is not strictly necessary as setPadding() and setVisibility() will
> trigger requestLayout() under the hood.

Removed.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abc25933673e