Adjust new tablet tabs tray button sizes

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mhaigh, Assigned: milo, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(3 attachments, 9 obsolete attachments)

Posted image spec_tabtray1.png
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
Flags: needinfo?(alam)
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..
Flags: needinfo?(alam)
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.
Assignee: mhaigh → nobody
Mentor: michael.l.comella, mhaigh
Whiteboard: [lang=java][good next bug]
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.
Flags: needinfo?(michael.l.comella)
(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!
Flags: needinfo?(michael.l.comella)
Assignee: nobody → d.lyons88
Any progress, Darren?
Flags: needinfo?(d.lyons88)
Unassigning due to inactivity. Darren, let me know if you still want to work on this.
Assignee: d.lyons88 → nobody
Flags: needinfo?(d.lyons88)
Posted patch Exploratory patch (obsolete) — Splinter Review
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?
Flags: needinfo?(michael.l.comella)
Posted patch Exploratory patch (fixed) (obsolete) — Splinter Review
Whoops, I uploaded the wrong patch. This should be correct.
Attachment #8616934 - Attachment is obsolete: true
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.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
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.
Posted image device-2015-06-09-142756.png (obsolete) —
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.)
Flags: needinfo?(alam)
Posted image device-2015-06-09-185327.png (obsolete) —
(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.
Flags: needinfo?(alam)
Looks good, is the (back) button set at 60dp width?
Flags: needinfo?(alam)
Posted patch Final patch (obsolete) — Splinter Review
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.
Attachment #8616937 - Attachment is obsolete: true
Attachment #8620509 - Flags: review?(michael.l.comella)
Assignee: nobody → m.lopiccolo3
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.
Attachment #8620509 - Flags: review?(mhaigh)
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.
Attachment #8620509 - Flags: review?(michael.l.comella)
Attachment #8620509 - Flags: review?(mhaigh)
Attachment #8620509 - Flags: feedback+
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?
Flags: needinfo?(mhaigh)
(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.
Attachment #8620509 - Flags: feedback+ → review+
Posted patch Patch for upper right buttons (obsolete) — Splinter Review
Here's a patch to fix the rest of the toolbar buttons.
Attachment #8623828 - Flags: review?(michael.l.comella)
Comment on attachment 8623828 [details] [diff] [review]
Patch for upper right buttons

Hang on - it wasn't complete. Back soon.
Attachment #8623828 - Attachment is obsolete: true
Attachment #8623828 - Flags: review?(michael.l.comella)
Posted image hitarea.png (obsolete) —
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.
Flags: needinfo?(alam)
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!
Flags: needinfo?(alam) → needinfo?(m.lopiccolo3)
Posted patch Patch for final review (obsolete) — Splinter 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!
Attachment #8620509 - Attachment is obsolete: true
Flags: needinfo?(m.lopiccolo3)
Attachment #8624340 - Flags: review?(michael.l.comella)
Attachment #8617666 - Attachment is obsolete: true
Attachment #8623836 - Attachment is obsolete: true
Comment on attachment 8624342 [details]
Appearance after applying attachment 8624340 [details] [diff] [review].

Anthony, does this look as expected?
Attachment #8624342 - Flags: feedback?(alam)
Comment on attachment 8624340 [details] [diff] [review]
Patch for final review

Martyn said he'd help out here.
Attachment #8624340 - Flags: review?(mhaigh)
Comment on attachment 8624342 [details]
Appearance after applying attachment 8624340 [details] [diff] [review].

Looks great!
Attachment #8624342 - Flags: feedback?(alam) → feedback+
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.
Attachment #8624340 - Flags: review?(mhaigh) → feedback+
Flags: needinfo?(mhaigh)
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.
Attachment #8624340 - Attachment is obsolete: true
Attachment #8624340 - Flags: review?(michael.l.comella)
Attachment #8626796 - Flags: review?(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"'
Attachment #8626796 - Flags: review?(mhaigh) → review+
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!
Flags: needinfo?(michael.l.comella)
(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
Flags: needinfo?(michael.l.comella) → needinfo?(m.lopiccolo3)
> Sure, can you post the bug number here? e.g. bug 784488

bug 1179793. Thank you!
Flags: needinfo?(m.lopiccolo3) → needinfo?(michael.l.comella)
All set!

By the way, for future use, you've also been granted the power to assign yourself to bugs. :)
Flags: needinfo?(michael.l.comella)
Run is green, so I'm setting this as checkin-needed.
Keywords: 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!
Flags: needinfo?(m.lopiccolo3)
Keywords: checkin-needed
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.
Attachment #8626796 - Attachment is obsolete: true
Flags: needinfo?(m.lopiccolo3)
Keywords: checkin-needed
The rebase was on mozilla-central, if that matters.
https://hg.mozilla.org/mozilla-central/rev/e29408a1923d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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.
You need to log in before you can comment on or make changes to this bug.