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

VERIFIED FIXED in Firefox 24

Status

()

Firefox for Android
Theme and Visual Design
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: Shilpan Bhagat)

Tracking

25 Branch
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24 verified, firefox25 verified, firefox26 verified, fennec24+)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 783136 [details]
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)
tracking-fennec: --- → ?
status-firefox23: --- → unaffected
status-firefox24: --- → affected
status-firefox25: --- → affected
Blocks: 872388
Shilpan - Talk to Wes if you need some context on this
Assignee: nobody → sbhagat
Duplicate of this bug: 884418
tracking-fennec: ? → 24+
(Assignee)

Comment 3

4 years ago
Any specifics on how large the width should be?
Flags: needinfo?(ibarlow)
try ~300dp
Flags: needinfo?(ibarlow)
(Assignee)

Comment 5

4 years ago
Created attachment 790965 [details] [diff] [review]
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.
Attachment #790965 - Flags: feedback?(wjohnston)
Shilpan, mind posting a screenshot?
(Assignee)

Comment 7

4 years ago
Created attachment 791405 [details]
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>
<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...
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?

Updated

4 years ago
Attachment #790965 - Flags: feedback?(ibarlow) → feedback+
Shilpan showed me a screenshot in IRC, I think the fixed size is fine.
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
This doesn't have r+ yet AFAICT.
Keywords: checkin-needed
r+ from me and landed:
https://hg.mozilla.org/integration/fx-team/rev/6366f575b4c6
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+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/c49c6d5059e6
status-firefox24: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a70b9450fc1
status-firefox25: affected → fixed
status-firefox26: --- → fixed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6366f575b4c6
Status: NEW → RESOLVED
Last Resolved: 4 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
status-firefox25: fixed → verified
status-firefox26: fixed → verified
Verified fixed on Firefox 24 Beta 8
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
(Reporter)

Updated

4 years ago
Depends on: 913402
You need to log in before you can comment on or make changes to this bug.