Closed Bug 1019318 Opened 6 years ago Closed 6 years ago

Polish button toast appearance

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
fennec 32+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(4 files)

Splitting this off from the conversation happening in bug 997288.

Right now antlam is talking about adding some transparency and tweaking the positioning. Want to make a mock-up?
Flags: needinfo?(alam)
Attached image preview_newtoast.png
Got a WIP for you guys right here!

I've shrunk the overall size of it so that the rounded corners can complete the "pill" shape on both ends. Also, I've added a transparency (~85%) to the toast matte and moved the positioning of the toast a little bit lower. The color is also less brown and taken from the tab tray background itself. 

It still covers the URL bar there and I'm still not happy about it but thought I'd post just to keep the ball rolling.

Thoughts?
Flags: needinfo?(alam)
Do we need to have separate styles for different Android versions? Or is this bug just an attempt to improve the most modern style? I haven't done an assessment of the different toast styles in different versions, so I'm not sure what we're working wiht here.

nalexander worked on the lastest sets of styles, so I want to know what he thinks.
Flags: needinfo?(nalexander)
(In reply to :Margaret Leibovic from comment #2)
> Do we need to have separate styles for different Android versions? Or is
> this bug just an attempt to improve the most modern style? I haven't done an
> assessment of the different toast styles in different versions, so I'm not
> sure what we're working wiht here.
> 
> nalexander worked on the lastest sets of styles, so I want to know what he
> thinks.

I think we do want to keep styles depending on Android version, because we mix toasts (styled per Android version, by Android) and button toasts (styled by us) throughout Fennec.

I followed the convention that wesj started, but implemented it in XML rather than in 9-patch PNGs.  My guess is that wesj found 9-patch PNGs in a good form to drop in, and that's a great way to get the platform-exact look.  I needed to really control padding and margins, and the 9-patch PNGs had different padding values depending on platform, so I replicated the PNGs, and later moved to Anthony's mocks, in XML.

That said, there are two axes at play:

Android version <v19 vs. v19+
Phone vs tablet

Technically, there's a shared StyleBase, and style Style with parent=StyleBase in each of the subdirectories.
Flags: needinfo?(nalexander)
I was just experimenting with the latest Android version here (the rounded ends) but I think we could adjust for the older versions of Android too, depending on what version the user is on.
tracking-fennec: ? → 32+
Assignee: nobody → margaret.leibovic
I will get to work on writing a patch for the latest styles, but in the meantime could we start brainstorming how this should work on older devices?

nalexander, how many sets of styles do we currently support? I'm hoping it's just two.
Flags: needinfo?(nalexander)
Flags: needinfo?(alam)
(In reply to Nick Alexander :nalexander from comment #3)
> I followed the convention that wesj started, but implemented it in XML
> rather than in 9-patch PNGs.  My guess is that wesj found 9-patch PNGs in a
> good form to drop in, and that's a great way to get the platform-exact look.

Yes. I was hoping to just steal the platform specific styles originally, but they don't fit buttons well :( So instead we just took the platform default drawables and modified them where we needed to. You could still use 9-patches for what you were doing. You'd just need to chop of the left side (right on rtl if we ever support it) and make sure they overlapped correctly. But the current stuff works fine too :)

Basically you two have hit problems because you're using icons, which we'd basically given up on in our other attempts. Personally, I still think you might be better to drop them here as well. They add visual clutter without much benefit (IMO), but this is the third time UX has wanted them in so I'm assuming they really want it :)
Attached image specs_snippet_mock1.png
Not sure if this helps, but I drew up what it would look like for the squarer toasts. I also took the liberty to try and draw up some specs around spacing and what not. Let me know if this helps..

These were done on an XXHDPI canvas and so the units would have to be converted as such.
Flags: needinfo?(alam)
(In reply to :Margaret Leibovic from comment #5)

> nalexander, how many sets of styles do we currently support? I'm hoping it's
> just two.

I'm sorry, you already answered this is comment 3.
Flags: needinfo?(nalexander)
Worked with antlam on IRC to come up with this. This makes the v19+/v19- styles a bit more similar, reduces the height of the toast to be more consistent with regular toasts, and uses a more black/transparent background.

Here's a build to test: https://dl.dropboxusercontent.com/u/3358452/fennec-toast-polish.apk

This is definitely looking good on my 4.4 devices, but unfortunately I don't have a 4.0-4.3 device with me to test. I tested on my 2.3 device, and there the button toast is a bit inconsistent with the regular toast, but it's actually more consistent than it was before this patch. I don't think we ever designed these button toasts for 2.3 (and I don't really care to optimize for that case).
Attachment #8438686 - Flags: review?(wjohnston)
Attachment #8438686 - Flags: review?(nalexander)
Comment on attachment 8438686 [details] [diff] [review]
Polish button toast appearance

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

::: mobile/android/base/resources/values-v19/styles.xml
@@ +14,5 @@
>          <item name="android:background">@null</item>
>          <item name="android:paddingLeft">24dp</item>
>          <item name="android:paddingRight">24dp</item>
> +        <item name="android:paddingTop">11dp</item>
> +        <item name="android:paddingBottom">11dp</item>

Oh, I just got rid of these two lines locally (since they match the parent style), but I forgot to qrefresh.
(In reply to :Margaret Leibovic from comment #10)
> Comment on attachment 8438686 [details] [diff] [review]
> Polish button toast appearance
> 
> Review of attachment 8438686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/values-v19/styles.xml
> @@ +14,5 @@
> >          <item name="android:background">@null</item>
> >          <item name="android:paddingLeft">24dp</item>
> >          <item name="android:paddingRight">24dp</item>
> > +        <item name="android:paddingTop">11dp</item>
> > +        <item name="android:paddingBottom">11dp</item>
> 
> Oh, I just got rid of these two lines locally (since they match the parent
> style), but I forgot to qrefresh.

Actually, I shouldn't have done that, it doesn't work!
Comment on attachment 8438686 [details] [diff] [review]
Polish button toast appearance

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

Code looks fine to me, and so does the APK.  I agree that you need the modified padding in both styles.xml -- *Base is not inheriting from anything common.
Attachment #8438686 - Flags: review?(nalexander) → review+
Attachment #8438686 - Flags: review?(wjohnston)
Attached image ICS screenshot
These don't have quite as much padding as the mockups. You can force a KitKat phone to show them if you want to.

Also, I probably would have also removed the item for non_active/checked button states entirely rather than just making it draw with a transparent color. I don't really trust that Android optimizes out the extra work there (and its junk code we don't need anymore).
(In reply to Wesley Johnston (:wesj) from comment #14)
> Created attachment 8439315 [details]
> ICS screenshot
> 
> These don't have quite as much padding as the mockups.

But do the mock-ups match the system toasts? I don't know that I like the extra padding just for button toasts :)

> You can force a
> KitKat phone to show them if you want to.

How do I do that?
 
> Also, I probably would have also removed the item for non_active/checked
> button states entirely rather than just making it draw with a transparent
> color. I don't really trust that Android optimizes out the extra work there
> (and its junk code we don't need anymore).

That wasn't working when I tried it. It appeared to me that you did need to keep the separate item. We could file a follow-up to try to optimize, though.
(In reply to :Margaret Leibovic from comment #15)
> But do the mock-ups match the system toasts? I don't know that I like the
> extra padding just for button toasts :)

These don't match system toasts anymore. We've thrown away the dropshadow. There's a subtle gradient on the normal ones. etc. I'm not worried about trying to match native anymore. Just trying to match mockups, or at least come to a result that makes UX happy. Up to antlam if he likes this I guess...

> How do I do that?
Dump the drawables/styles into the kitkat folder, or delete the kitkat resources so that we'll use the more generic ones.

> That wasn't working when I tried it. It appeared to me that you did need to
> keep the separate item. We could file a follow-up to try to optimize, though.
Hmm... I tried this locally and it worked.
(In reply to Wesley Johnston (:wesj) from comment #16)
> (In reply to :Margaret Leibovic from comment #15)
> > But do the mock-ups match the system toasts? I don't know that I like the
> > extra padding just for button toasts :)
> 
> These don't match system toasts anymore. We've thrown away the dropshadow.
> There's a subtle gradient on the normal ones. etc. I'm not worried about
> trying to match native anymore. Just trying to match mockups, or at least
> come to a result that makes UX happy. Up to antlam if he likes this I
> guess...

I don't care about matching directly, but I do think that part of the problem here was that these toasts were way too tall.

antlam and I didn't explicitly discuss the height for ICS styles, but he did say that he was fine with them being more similar to what we're doing for v19+ (such as the background colors).

> > How do I do that?
> Dump the drawables/styles into the kitkat folder, or delete the kitkat
> resources so that we'll use the more generic ones.

Oh, but that wouldn't change the style of the system toasts, which is what I wanted to compare to.

I tested this on a 2.3 phone, so I have seen what these styles look like on a device.
(In reply to :Margaret Leibovic from comment #17)
> (In reply to Wesley Johnston (:wesj) from comment #16)
> > (In reply to :Margaret Leibovic from comment #15)
> > > But do the mock-ups match the system toasts? I don't know that I like the
> > > extra padding just for button toasts :)
> > 
> > These don't match system toasts anymore. We've thrown away the dropshadow.
> > There's a subtle gradient on the normal ones. etc. I'm not worried about
> > trying to match native anymore. Just trying to match mockups, or at least
> > come to a result that makes UX happy. Up to antlam if he likes this I
> > guess...
> 
> I don't care about matching directly, but I do think that part of the
> problem here was that these toasts were way too tall.
> 
> antlam and I didn't explicitly discuss the height for ICS styles, but he did
> say that he was fine with them being more similar to what we're doing for
> v19+ (such as the background colors).

These toasts look fine to me. I'm not too worried about conforming completely from a visual standpoint because the experience of these toasts (where they appear, how they function, etc) translate pretty well.

From a usability standpoint, I can see these maybe being too short and hard to tap? But since this is the height of the native ones for >v19 then we should just keep it.
https://hg.mozilla.org/mozilla-central/rev/c7457e8f1cce
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Depends on: 1026798
No longer depends on: 1026798
You need to log in before you can comment on or make changes to this bug.