1105271 - Adjust new tablet tabs tray button sizes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Attachment: spec_tabtray1.png

Adjust button sizes to follow attached spec

Comment 1

[Martyn Haigh (:mhaigh)]

antlam: can we get an idea of how this should look with the back button defined in bug 1100464

Comment 2

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

Lucas mentioned this would be hard to do as a part of V1. So we'll have to get this done in V2 I think..

Comment 3

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

Gist of it is, the back button hit area is the same as the dark purple in the spec. It then inserts to the left of the "tab" and "private tab" buttons there. It should also ignore the left margin since there's enough space created by the padding of the icon inside the button itself.

Comment 5

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

A good place to start looking would be TabsPanel for the whole container and TabsGridLayout for the inner thumbnail view.

Comment 6

[Darren Lyons]

mcomella: I'll take a look at this bug Michael.

Comment 7

[Darren Lyons]

What actually needs to be changed? Not sure I understand the bug description, sorry.

Comment 8

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

(In reply to Darren Lyons from comment #7)
> What actually needs to be changed? Not sure I understand the bug
> description, sorry.

The goal is to set up the View size and spacing according to the attached spec. You might want to start digging in the TabsPanel class:

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

Let me know if you have further questions!

Comment 9

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

Any progress, Darren?

Comment 10

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

Unassigning due to inactivity. Darren, let me know if you still want to work on this.

Comment 11

[Michael LoPiccolo (:milo)]

Attachment: Exploratory patch

I've been looking at this today and I wanted to make sure that I was on the right track.
The width of the buttons is determined by "tabs_panel_indicator_width" in dimens.xml, so I changed it to 72dp for large screens and up (which should cover all tablets).

Behavior works as expected on my tablet and phone, but there were still some things I wanted to ask about:

-What about the back button? Right now, it has the same size as the other two buttons. Is that the goal? If it has to have a different size, I'll have to find some way to dynamically set the buttons' sizes.

-What about older devices? There are no resources which are large but not large-v11 or higher. Are tablets below v11 just not supported, or should I make a new file?

Comment 12

[Michael LoPiccolo (:milo)]

Attachment: Exploratory patch (fixed)

Whoops, I uploaded the wrong patch. This should be correct.

Comment 13

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

Anthony, were there new resources associated with this change?

(In reply to Michael LoPiccolo (:milo) from comment #11)
> -What about the back button? Right now, it has the same size as the other
> two buttons. Is that the goal? If it has to have a different size, I'll have
> to find some way to dynamically set the buttons' sizes.

Anthony, should the back button still follow the guidelines you mentioned in comment 3??

> -What about older devices? There are no resources which are large but not
> large-v11 or higher. Are tablets below v11 just not supported, or should I
> make a new file?

API 11 introduced tablets so there are no tablets below v11.

Comment 14

[Richard Newman [:rnewman]]

That's not strictly true:

https://encrypted.google.com/search?q=gingerbread+tablet

but we don't put a lot of effort into supporting them.

Comment 15

[Michael LoPiccolo (:milo)]

Attachment: device-2015-06-09-142756.png

This is the way it looks on my device right now. If I understand comment 3 correctly, the back button should be shrunk back to a width of 60dp.

Comment 16

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

(In reply to Michael LoPiccolo (:milo) from comment #15)
> Created attachment 8617520 [details]
> device-2015-06-09-142756.png
> 
> This is the way it looks on my device right now. If I understand comment 3
> correctly, the back button should be shrunk back to a width of 60dp.

Correct! can we shrink the width of the entire button to be 60dp? (60 dp width x 48 dp height with the icon centered inside this hit area.)

Comment 17

[Michael LoPiccolo (:milo)]

Attachment: device-2015-06-09-185327.png

(In reply to Anthony Lam (:antlam) from comment #16)
> Correct! can we shrink the width of the entire button to be 60dp? (60 dp
> width x 48 dp height with the icon centered inside this hit area.)

I think I figured it out. Does this new screenshot look right, Anthony?
If so, I'll put out a patch, with less magic numbers this time.

Comment 18

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

Looks good, is the (back) button set at 60dp width?

Comment 19

[Michael LoPiccolo (:milo)]

Attachment: Final patch

Alright, here's the final patch. The appearance on my tablet is still the same as the last screenshot, and the appearance on my phone hasn't changed.

I created a new dimen value, tabs_panel_back_button_width, so that the back button and other buttons could be different sizes. Let me know if there's a cleaner way to do it.

I wasn't sure who to request the review from. Michael, feel free to pass it on to someone else if that works better.

Comment 20

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

Comment on attachment 8620509 [details] [diff] [review]
Final patch

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

Sorry for the delay, Michael – I've done a poor job juggling my workload. I added Martyn as a reviewer if I don't get to it today.

Comment 21

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

Comment on attachment 8620509 [details] [diff] [review]
Final patch

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

Michael, have you looked into making sure the other tabs tray button sizes are also correct in the mock? There are no changes for the menu button that I can see, and the menu button doesn't appear to be 60dp at the moment:

https://mxr.mozilla.org/mozilla-central/search?string=[^0-9]60dp&regexp=on&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

But maybe I missed it (i.e. it's defined as padding).

Martyn, can you try this on device? I don't have a tablet with me.

Comment 22

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

For the record, it is my impression that the tabs tray buttons for this bug are the top bar (back to menu) – we can file another bug to verify the remaining objects are correct (because I think those are trickier as they're sized dynamically).

Martyn (here & comment 21), are the tabs tray thumbnails sized dynamically?

Comment 23

[Michael LoPiccolo (:milo)]

(In reply to Michael Comella (:mcomella) from comment #21)
> Comment on attachment 8620509 [details] [diff] [review]
> Final patch
> 
> Review of attachment 8620509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Michael, have you looked into making sure the other tabs tray button sizes
> are also correct in the mock? There are no changes for the menu button that
> I can see, and the menu button doesn't appear to be 60dp at the moment:

I haven't looked at the buttons in the top right, you're correct. I'll look into it today and if it's not trivial I'll split it off into a new bug?

Comment 24

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

(In reply to Michael LoPiccolo (:milo) from comment #23)
> I haven't looked at the buttons in the top right, you're correct. I'll look
> into it today and if it's not trivial I'll split it off into a new bug?

WFM.

Comment 25

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

Comment on attachment 8620509 [details] [diff] [review]
Final patch

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

r+ assuming another patch or a follow-up to land in the same version.

Comment 26

[Michael LoPiccolo (:milo)]

Attachment: Patch for upper right buttons

Here's a patch to fix the rest of the toolbar buttons.

Comment 27

[Michael LoPiccolo (:milo)]

Comment on attachment 8623828 [details] [diff] [review]
Patch for upper right buttons

Hang on - it wasn't complete. Back soon.

Comment 28

[Michael LoPiccolo (:milo)]

Attachment: hitarea.png

The patch I uploaded isn't ready for review (sorry Michael), but I wanted to get some UX feedback and see if I was on the right track.

Alright. This screenshot is how it looks when I'm pressing the menu button. To me, the 6dp margin in the spec makes this look somewhat off.

Anthony - Did I misinterpret the spec, does the spec need to be changed, or is this the intended appearance?

If it were up to me, I would just remove the margin. It doesn't fit the feel of other Android apps, imo.

Comment 29

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

Hey Milo!

Yep, you can go ahead and remove it. I think this was an old spec. I can't find that margin in my mocks anymore :)

thanks!

Comment 30

[Michael LoPiccolo (:milo)]

Attachment: Patch for final review

Alright, the margin is gone and everything's ready to go (assuming I didn't donk up Mercurial again).

This patch expands and obsoletes attachment 8620509 [details] [diff] [review].

Tested on my LG G Pad 7.0 (I'll upload a screenshot next). I made sure I hadn't changed the phone UI by testing on my Sony Xperia Z3 Compact. I didn't run Robocop tests, let me know if I need to.

I believe it should be ready to upload to the try server!

Comment 31

[Michael LoPiccolo (:milo)]

Attachment: Appearance after applying attachment 8624340.

Comment 32

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

Comment on attachment 8624342 [details]
Appearance after applying attachment 8624340 [details] [diff] [review].

Anthony, does this look as expected?

Comment 33

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

Comment on attachment 8624340 [details] [diff] [review]
Patch for final review

Martyn said he'd help out here.

Comment 34

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

Comment on attachment 8624342 [details]
Appearance after applying attachment 8624340 [details] [diff] [review].

Looks great!

Comment 35

[Martyn Haigh (:mhaigh)]

Comment on attachment 8624340 [details] [diff] [review]
Patch for final review

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

Looks good but there seems to be something weird going on with your patch file.  mobile/android/base/resources/layout/tabs_panel_default.xml has two entries - the first introduces hardcoded widths and the second replaces those with dimens.  The patch itself is fine but I'd like to see this issue sorted before we push it.  Submit a new patch and I'll review again.

Comment 36

[Michael LoPiccolo (:milo)]

Attachment: Patch for final review, addressed feedback

Thanks for the feedback, Martyn!
I fixed it after some Mercurial haggling. Tested on both my devices and the behavior is still correct.
In the future, I'll make sure my patches only span one commit.

Comment 37

[Martyn Haigh (:mhaigh)]

Comment on attachment 8626796 [details] [diff] [review]
Patch for final review, addressed feedback

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

Looks good to me (LGTM)

Before you push, it's good practise to push to our try server to make sure that your proposed patch doesn't break anything once you push to the main repo.  I don't know if you've pushed to try before, but here are some resources:
1, https://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/extensions.html#trychooser
2, https://developer.mozilla.org/en/docs/Installing_Mercurial#Configuring_the_try_repository
3, https://wiki.mozilla.org/ReleaseEngineering/TryServer

once you have the hg push-to-try extension installed, you can use mcomella's bash alias which makes it really easy to push in the future:

alias htry='hg push-to-try -m "try: -b do -p android-api-9,android-api-11,android-x86 -u robocop -t none"'

Comment 38

[Michael LoPiccolo (:milo)]

mcomella: Would you be willing to vouch me for level 1 commit access so I can push this to try? (If you're not ready to vouch yet, could you push to try for me?) Thanks!

Comment 39

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

(In reply to Michael LoPiccolo (:milo) from comment #38)
> mcomella: Would you be willing to vouch me for level 1 commit access so I
> can push this to try? (If you're not ready to vouch yet, could you push to
> try for me?) Thanks!

Sure, can you post the bug number here? e.g. bug 784488

Comment 40

[Michael LoPiccolo (:milo)]

> Sure, can you post the bug number here? e.g. bug 784488

bug 1179793. Thank you!

Comment 41

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

All set!

By the way, for future use, you've also been granted the power to assign yourself to bugs. :)

Comment 42

[Michael LoPiccolo (:milo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd8a07353d3d

Comment 43

[Michael LoPiccolo (:milo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc61b890acb3

Comment 44

[Michael LoPiccolo (:milo)]

Run is green, so I'm setting this as checkin-needed.

Comment 45

[Carsten Book [:Tomcat]]

sorry failed to apply: applying patch
patching file mobile/android/base/resources/layout/tabs_panel_default.xml
Hunk #1 FAILED at 31
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/layout/tabs_panel_default.xml.rej
patching file mobile/android/base/resources/values-large-v11/dimens.xml
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values-large-v11/dimens.xml.rej
patching file mobile/android/base/resources/values-xlarge-v11/dimens.xml
Hunk #1 FAILED at 3
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values-xlarge-v11/dimens.xml.rej
patching file mobile/android/base/resources/values/dimens.xml
Hunk #1 FAILED at 133
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/dimens.xml.rej

can you take a look, thanks!

Comment 46

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

Hey, Michael. It looks like the patch failed to apply on the latest fx-team.

Can you rebase your patch onto the latest fx-team (or mozilla-central, depending on your checkout)? If you have questions about how to do this, let me know!

Comment 47

[Michael LoPiccolo (:milo)]

Attachment: Rebased final patch

Attached the rebased patch. I'll set checkin-needed, but let me know if it needs to be re-reviewed, re-tested, or if there's anything else I missed.

Comment 48

[Michael LoPiccolo (:milo)]

The rebase was on mozilla-central, if that matters.

Comment 49

[Pulsebot]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

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

Comment 51

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

Hey, Michael, if you're interested in some follow-up bugs, perhaps bug 990042 for front-end, bug 1158994 for a front-end challenge, or bug 1130809 for back-end.