899560 - [Tablet] - Excessively large bookmark added notification toast on tablets

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (07/30) - Screenshot

See screenshot.

Bug 872388 added 'super-toasts' functionality for bookmarking via our menu. They appear fine on phones but deserve a width constraint on landscape tablets. 

--
Samsung Galaxy Tab 3 10" (Android 4.2)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Shilpan - Talk to Wes if you need some context on this

Comment 3

[Shilpan Bhagat]

Any specifics on how large the width should be?

Comment 4

[Ian Barlow (:ibarlow)]

try ~300dp

Comment 5

[Shilpan Bhagat]

Attachment: Patch for smaller toasts

After trying out a lot of different ways of doing this, I finally resorted to have a different width style for toasts on tablets as compared to phones. 

We can have a larger toast on tablets in my opinion. ~500dp should be safe enough.

Comment 6

[Ian Barlow (:ibarlow)]

Shilpan, mind posting a screenshot?

Comment 7

[Shilpan Bhagat]

Attachment: Screenshot - tablet

With 300dp

Comment 8

[Ian Barlow (:ibarlow)]

Looks good!

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 790965 [details] [diff] [review]
Patch for smaller toasts

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

This fixes the width of the toast at 300dp on xlarge screen devices (i.e. it won't stretch to any larger). I just want ian's feedback on that and some comments added to the patch.

::: mobile/android/base/resources/values-large-v11/styles.xml
@@ +65,5 @@
>      <style name="TabsItemClose">
>           <item name="android:nextFocusUp">@+id/info</item>
>      </style>
>  
> +    <style name="Toast">

Sorry for the delay here. I wish we didn't have to override everything for this to work. Maybe just a way to make it more obvious what's different for this style.

I think the right way to do this is probably to split out shared theme parts and try to only override what's necessary here. That probably means this needs to be something like

<style name="Toast">
  ... shared stuff
</style>
<style name="Toast.DeviceSpecific">
  ... device specific stuff?
</style>

That feels like overkill I think though, so maybe we're better just commenting this extensively so that its obvious what's different here. i.e. even something like:

<style name="Toast">
  ... devices specific stuff...
  // If you change anything below here, also update it in values/styles.xml
  .. shared stuff...

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

We need to push this in 24 if at all possible. Let's get past feedback and into review.

Comment 11

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #9)

> This fixes the width of the toast at 300dp on xlarge screen devices (i.e. it
> won't stretch to any larger). I just want ian's feedback on that and some
> comments added to the patch.

Can I see a screenshot of that?

Comment 12

[Ian Barlow (:ibarlow)]

Shilpan showed me a screenshot in IRC, I think the fixed size is fine.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This doesn't have r+ yet AFAICT.

Comment 14

[Wesley Johnston (:wesj)]

r+ from me and landed:
https://hg.mozilla.org/integration/fx-team/rev/6366f575b4c6

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 790965 [details] [diff] [review]
Patch for smaller toasts

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 872388
User impact if declined: This one particular toast is pretty ugly
Testing completed (on m-c, etc.): Landed on mc today
Risk to taking this patch (and alternatives if risky): very low risk. Just a style change for tablets.
String or IDL/UUID changes made by this patch: none.

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 790965 [details] [diff] [review]
Patch for smaller toasts

Approving as this is a low risk cosmetic change.Pease make sure to land by Thursday morning PT so it get into our second last mobile beta.

Comment 17

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c49c6d5059e6

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a70b9450fc1

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6366f575b4c6

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 26.0a1 (2013-09-05) and Firefox for Android 25.0a2 (2013-09-05) 
Device: Asus Transformer TF 101 
OS: Android 4.0.4

Comment 21

[Catalin Suciu [:csuciu]]

Verified fixed on Firefox 24 Beta 8