Adjust button sizes to follow attached spec

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

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

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.

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

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

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

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

Any progress, Darren?

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

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?

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

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.

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.

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.

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

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

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

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

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?

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

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

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

Comment on attachment 8623828 [details] [diff] [review] Patch for upper right buttons Hang on - it wasn't complete. Back soon.

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.

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!

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 on attachment 8624342 [details] Appearance after applying attachment 8624340 [details] [diff] [review]. Anthony, does this look as expected?

Comment on attachment 8624340 [details] [diff] [review] Patch for final review Martyn said he'd help out here.

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

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.

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 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"'

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!

(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

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

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

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

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!

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!

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.

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

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.