Closed Bug 899560 Opened 7 years ago Closed 7 years ago

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


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

25 Branch
Not set



Firefox 26
Tracking Status
firefox23 --- unaffected
firefox24 --- verified
firefox25 --- verified
firefox26 --- verified
fennec 24+ ---


(Reporter: aaronmt, Assigned: shilpanbhagat)




(3 files)

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)
tracking-fennec: --- → ?
Blocks: 872388
Shilpan - Talk to Wes if you need some context on this
Assignee: nobody → sbhagat
Duplicate of this bug: 884418
tracking-fennec: ? → 24+
Any specifics on how large the width should be?
Flags: needinfo?(ibarlow)
try ~300dp
Flags: needinfo?(ibarlow)
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.
Attachment #790965 - Flags: feedback?(wjohnston)
Shilpan, mind posting a screenshot?
Attached image Screenshot - tablet
With 300dp
Flags: needinfo?(ibarlow)
Looks good!
Flags: needinfo?(ibarlow)
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 name="Toast.DeviceSpecific">
  ... device specific stuff?

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...
Attachment #790965 - Flags: feedback?(wjohnston)
Attachment #790965 - Flags: feedback?(ibarlow)
Attachment #790965 - Flags: feedback+
We need to push this in 24 if at all possible. Let's get past feedback and into review.
(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?
Attachment #790965 - Flags: feedback?(ibarlow) → feedback+
Shilpan showed me a screenshot in IRC, I think the fixed size is fine.
Keywords: checkin-needed
This doesn't have r+ yet AFAICT.
Keywords: checkin-needed
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.
Attachment #790965 - Flags: review+
Attachment #790965 - Flags: feedback+
Attachment #790965 - Flags: approval-mozilla-beta?
Attachment #790965 - Flags: approval-mozilla-aurora?
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.
Attachment #790965 - Flags: approval-mozilla-beta?
Attachment #790965 - Flags: approval-mozilla-beta+
Attachment #790965 - Flags: approval-mozilla-aurora?
Attachment #790965 - Flags: approval-mozilla-aurora+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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
Verified fixed on Firefox 24 Beta 8
Depends on: 913402
You need to log in before you can comment on or make changes to this bug.