1130203 - Implement v2 share overlay mock

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The heavy lifting for bug 1130188.

mock: https://bug1059554.bugzilla.mozilla.org/attachment.cgi?id=8549889

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, can I get some values for the mock linked in comment 0?

In particular, I'm doing font size, style, and distance between lines.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1130203/mcomella

/r/3439 - Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh
/r/3441 - Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh

Pull down these commits:

hg pull review -r dd9e23202d096de575456ba88f45bb865abd84b8

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: changes from comment 2 are not yet final.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_shareoverlay_mock4b.png

Attaching mocks for the time being, will get you specs when I'm back in the right time zone ;)

FWIW: 

Page title: Roboto Light, 20sp, white
URL: Robobo Regular, 12sp, white (might be too small and we probably have to be smart about this info here).
Buttons text: Roboto Regular, 14sp, #363B40
Buttons text disabled: #BFBFBF

Padding: 20 dp from top of the overlay, 8 dp in between Page Title and URL (again, maybe too small depending on type size of URL)

Hope this quick spec was at least a little bit helpful...

Comment 5

[Martyn Haigh (:mhaigh)]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

https://reviewboard.mozilla.org/r/3437/#review2815

Looking good.

::: mobile/android/base/resources/layout/overlay_share_dialog.xml
(Diff revision 1)
> -                android:textColor="@color/text_color_primary"
> +            android:textSize="24sp"/>

Would be good to get this in to a style; perhaps a Title and Subtitle style makes sense?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #5)
> Would be good to get this in to a style; perhaps a Title and Subtitle style
> makes sense?

Why is that?

When I first looked at the file, I thought, "omg nothing is in a style", but then I realized that none of these styles needed to be reused and putting them into a style was just making them harder to edit.

Comment 7

[Martyn Haigh (:mhaigh)]

Ideally I'd like to work towards having one single source of truth for our styles.  It'll make them easier to manage, easier to get an overview of what styles we have and hence easier to refactor (and actually see that we can/need to refactor).  I don't we should shy away from putting single use styles in the styles.xml file, it'll help us form a cohesive style strategy which we really need.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: WIP

Anthony, I think we should re-style the device list - what do you think? Probably center the device icon and shift the device name to be in line with the "Add to Reading List" and such.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, also how wide should the dialog be?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And what color should the pressed & focused states be?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #10)
> And what color should the pressed

Found #F5F5F5. Correcty?

> & focused states be?

Maybe we should reduce complexity by just not specifying.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8570282 [details]
WIP

Will attach specs :)

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_shareoverlay1.png

These should help

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: shareoverlay2_icons.zip

Also, found these (which I think are also in the other bugs)

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_greencheck.zip

Adding check (forgot!)

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

/r/3439 - Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh
/r/3441 - Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh
/r/4469 - Bug 1130203 - Remove Firefox logo from share overlay. r=mhaigh
/r/4471 - Bug 1130203 - Move share overlay title styles into styles.xml and revise to match mocks. r=mhaigh
/r/4473 - Bug 1130203 - Remove dividers in share overlay. r=mhaigh
/r/4475 - Bug 1130203 - Add @dimen/button_corner_radius and replace corner radius use in code. r=mhaigh
/r/4477 - Bug 1130203 - Round the corners of the first item in the share overlay. r=mhaigh
/r/4479 - Bug 1130203 - Set width for share overlay. r=mhaigh

Pull down these commits:

hg pull review -r 6907ab74691c06c2f65e9d8c7c708d09b01807f7

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still TODO:
  * (followup?) Clean up styles
  * (followup?) Add back drop shadow (liuche mentioned she was doing a bug on this)

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Oh, and making sure the styles of the menu items are consistent (images *and* text), particularly the devices to share to.

Anthony, any specific assets for the devices to share to?

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot (post commits)

TODOs mentioned above this comment - what do you think, Anthony?

Comment 20

[Martyn Haigh (:mhaigh)]

https://reviewboard.mozilla.org/r/4479/#review3677

Ship It!

Comment 21

[Martyn Haigh (:mhaigh)]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

https://reviewboard.mozilla.org/r/3437/#review3679

Ship It!

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8570826 [details]
Screenshot (post commits)

Looking good! We'll also need to update that "Add to RL" icon there

Comment 23

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^Oh, text should be #363B40 (text and tabs tray grey) right? looks a bit light there.

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: shareoverlay3_icons.zip

Included the "Add to Reading List" icon and updated colors. Will also mark obsolete all icons in bug 1059554

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #17)
> Still TODO:
>   * (followup?) Clean up styles
>   * (followup?) Add back drop shadow (liuche mentioned she was doing a bug
> on this)

imo, neither of these are important enough to block 38 - I'll do the updates in comment 23 and comment 24 and see what antlam thinks.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

/r/3439 - Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh
/r/3441 - Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh
/r/4469 - Bug 1130203 - Remove Firefox logo from share overlay. r=mhaigh
/r/4471 - Bug 1130203 - Move share overlay title styles into styles.xml and revise to match mocks. r=mhaigh
/r/4473 - Bug 1130203 - Remove dividers in share overlay. r=mhaigh
/r/4475 - Bug 1130203 - Add @dimen/button_corner_radius and replace corner radius use in code. r=mhaigh
/r/4477 - Bug 1130203 - Round the corners of the first item in the share overlay. r=mhaigh
/r/4479 - Bug 1130203 - Set width for share overlay. r=mhaigh
/r/5683 - Bug 1130203 - Update share overlay text colors to match mocks. r=mhaigh
/r/5685 - Bug 1130203 - Rename TextAppearance.ShareOverlay to ShareOverlayTextAppearance. r=mhaigh
/r/5687 - Bug 1130302 - Move ShareOverlayButton.Text to ShareOverlayTextAppearance.Button. r=mhaigh
/r/5689 - Bug 1130203 - Remove excess LinearLayout from ShareOverlay. r=mhaigh
/r/5691 - Bug 1130203 - Remove unused share overlay layout. r=mhaigh
/r/5693 - Bug 1130203 - Clean up style inheritance in share overlay. r=mhaigh
/r/5695 - Bug 1130203 - Update share overlay row pressed color & color names. r=mhaigh

Pull down these commits:

hg pull review -r d710485fafed966fca96676930fd48c857daabde

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still TODO:
  * On L, we may have to reset the drawable that uses the first row
  * Setting the style for the row image (and include the new assets)
  * Clean up the success toast styles

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Already added to Reading List icon plz!

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_RLed.zip

Here ya go

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Success toast

As discussed with antlam irl, it's hard to do the proposed success mock because the share action happens asynchronously so we're just prettying up the existing share dialog.

What do you think, Anthony? I think we need a new background color.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Failure toast (no retry)

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Toast (retry)

Since we don't plan to show this much, I'm fine that it looks kind of ugly.

A little concerned over what will happen if there's too much wrapping going on though.

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

/r/3439 - Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh
/r/3441 - Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh
/r/4469 - Bug 1130203 - Remove Firefox logo from share overlay. r=mhaigh
/r/4471 - Bug 1130203 - Move share overlay title styles into styles.xml and revise to match mocks. r=mhaigh
/r/4473 - Bug 1130203 - Remove dividers in share overlay. r=mhaigh
/r/4475 - Bug 1130203 - Add @dimen/button_corner_radius and replace corner radius use in code. r=mhaigh
/r/4477 - Bug 1130203 - Round the corners of the first item in the share overlay. r=mhaigh
/r/4479 - Bug 1130203 - Set width for share overlay. r=mhaigh
/r/5683 - Bug 1130203 - Update share overlay text colors to match mocks. r=mhaigh
/r/5685 - Bug 1130203 - Rename TextAppearance.ShareOverlay to ShareOverlayTextAppearance. r=mhaigh
/r/5687 - Bug 1130302 - Move ShareOverlayButton.Text to ShareOverlayTextAppearance.Button. r=mhaigh
/r/5689 - Bug 1130203 - Remove excess LinearLayout from ShareOverlay. r=mhaigh
/r/5691 - Bug 1130203 - Remove unused share overlay layout. r=mhaigh
/r/5693 - Bug 1130203 - Clean up style inheritance in share overlay. r=mhaigh
/r/5695 - Bug 1130203 - Update share overlay row pressed color & color names. r=mhaigh
/r/5777 - Bug 1130203 - Update ShareOverlay icon padding & assets. r=mhaigh
/r/5779 - Bug 1130203 - Clean up share overlay toast styles. r=mhaigh
/r/5781 - Bug 1130203 - Reset the first item background drawable state onResume. r=mhaigh

Pull down these commits:

hg pull review -r b1403998beb5bcd47a3cfbb46c67f910dd346eb5

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

comment 33 should be final pending antlam approval (until we revise the toast at a later point).

Comment 35

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8580380 [details]
Success toast

Let's capitalize the "T" in "Tab". 

I think the color is fine if we can get a drop shadow in there. If not, it will completely depend on the content background of the site (dark vs. light). In which case, using our Toolbar Grey is probably just as good.

Comment 36

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

For the "try again" one. Let's do something even simpler. 

Can we try center aligning the text and replacing the string with "There was a problem sending your Tab, try again?" - and have "try again?" in our "Link blue"?

That should be enough and we can keep it more consistent with our other visual styles :)

Comment 37

[Martyn Haigh (:mhaigh)]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

https://reviewboard.mozilla.org/r/3437/#review4837

Looks good, but I'd rather see us inlining styles if they aren't used on more than one element with the same symantic meaning (http://blog.danlew.net/2014/11/19/styles-on-android/)

::: mobile/android/base/resources/drawable/firstrun_button_enabled.xml
(Diff revision 4)
> -      android:radius="@dimen/fxaccount_corner_radius" />
> +      android:radius="@dimen/button_corner_radius" />

Yay - Lets make everything generic!  Will this work for all future button corners?

::: mobile/android/base/resources/layout/overlay_share_button.xml
(Diff revision 4)
> -        style="@style/ShareOverlayButton.Text"
> +        android:textAppearance="@style/ShareOverlayTextAppearance.Button"

Is this style used in more than one place?  If not I'd rather have them inline

::: mobile/android/base/resources/layout/overlay_share_dialog.xml
(Diff revision 4)
> +            android:textAppearance="@style/ShareOverlayTextAppearance.Header.Title"

As above - lets reduce the number of styles we have with only one user.

::: mobile/android/base/resources/layout/overlay_share_dialog.xml
(Diff revision 4)
> +        <!-- TODO: Add back drop shadow? -->

Maybe add a TODO with a bug number?

::: mobile/android/base/resources/layout/overlay_share_send_tab_item.xml
(Diff revision 4)
> +    style="@style/ShareOverlayRow"

nit: align attributes?

::: mobile/android/base/resources/layout/overlay_share_toast.xml
(Diff revision 4)
> -</LinearLayout>

This file is much nicer now!

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #35)
> Let's capitalize the "T" in "Tab". 

I'm not sure I agree with this one - why do you think it should be capitalized?

e.g. desktop prefs has, in the Tabs section, "Open new windows in a new tab instead"

> I think the color is fine if we can get a drop shadow in there.

I'll try using liuche's method.

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/3437/#review4841

> Yay - Lets make everything generic!  Will this work for all future button corners?

Yep - it's just a dimen.

We can probably make the button a generic resource too (e.g. color_button_round), but maybe that should be a follow-up cleanup effort.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #36)
> Can we try center aligning the text and replacing the string with "There was
> a problem sending your Tab, try again?" - and have "try again?" in our "Link
> blue"?

One problem is that we can't uplift the String - are you fine having my current "Try again?" popup in 38, and having this cleaner version in 39? As an error case, it shouldn't be seen very often so it should be fine.

Comment 41

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #38)
> (In reply to Anthony Lam (:antlam) from comment #35)
> > Let's capitalize the "T" in "Tab". 
> 
> I'm not sure I agree with this one - why do you think it should be
> capitalized?
> 
> e.g. desktop prefs has, in the Tabs section, "Open new windows in a new tab
> instead"

It should be capitalized because we capitalize it in all our products (Desktop, Mobile, etc) so I want to keep it consistent. Basically, it's a name for our product.


(In reply to Michael Comella (:mcomella) from comment #40)
> (In reply to Anthony Lam (:antlam) from comment #36)
> > Can we try center aligning the text and replacing the string with "There was
> > a problem sending your Tab, try again?" - and have "try again?" in our "Link
> > blue"?
> 
> One problem is that we can't uplift the String - are you fine having my
> current "Try again?" popup in 38, and having this cleaner version in 39? As
> an error case, it shouldn't be seen very often so it should be fine.

So this is what we currently have in our product? Can we remove the button shape/color then and just have text?

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #41)
> > One problem is that we can't uplift the String - are you fine having my
> > current "Try again?" popup in 38, and having this cleaner version in 39? As
> > an error case, it shouldn't be seen very often so it should be fine.
> 
> So this is what we currently have in our product? 

Functionality, yes; styles, no.

> Can we remove the button
> shape/color then and just have text?

Actually, I didn't realize the string didn't change so yeah, we can do this.

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Success toast w/ dropshadow

Comment 44

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8582140 [details]
Success toast w/ dropshadow

Niiiiice!

Comment 45

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Failure toast

Pretty simple.

Comment 46

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

/r/3439 - Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh
/r/3441 - Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh
/r/4469 - Bug 1130203 - Remove Firefox logo from share overlay. r=mhaigh
/r/4471 - Bug 1130203 - Move share overlay title styles into styles.xml and revise to match mocks. r=mhaigh
/r/4473 - Bug 1130203 - Remove dividers in share overlay. r=mhaigh
/r/4475 - Bug 1130203 - Add @dimen/button_corner_radius and replace corner radius use in code. r=mhaigh
/r/4477 - Bug 1130203 - Round the corners of the first item in the share overlay. r=mhaigh
/r/4479 - Bug 1130203 - Set width for share overlay. r=mhaigh
/r/5683 - Bug 1130203 - Update share overlay text colors to match mocks. r=mhaigh
/r/5685 - Bug 1130203 - Rename TextAppearance.ShareOverlay to ShareOverlayTextAppearance. r=mhaigh
/r/5687 - Bug 1130302 - Move ShareOverlayButton.Text to ShareOverlayTextAppearance.Button. r=mhaigh
/r/5689 - Bug 1130203 - Remove excess LinearLayout from ShareOverlay. r=mhaigh
/r/5691 - Bug 1130203 - Remove unused share overlay layout. r=mhaigh
/r/5693 - Bug 1130203 - Clean up style inheritance in share overlay. r=mhaigh
/r/5695 - Bug 1130203 - Update share overlay row pressed color & color names. r=mhaigh
/r/5777 - Bug 1130203 - Update ShareOverlay icon padding & assets. r=mhaigh
/r/5779 - Bug 1130203 - Clean up share overlay toast styles. r=mhaigh
/r/5781 - Bug 1130203 - Reset the first item background drawable state onResume. r=mhaigh
/r/5997 - Bug 1130203 - Review: Remove single use styles in share overlay. r=trivial
/r/5999 - Bug 1130203 - Review: Finish off share overlay nits. r=trivial
/r/6001 - Bug 1130203 - Add drop shadow to overlay share dialog result toast. r=mhaigh
/r/6003 - Bug 1130203 - Remove retry button in share overlay retry toast. r=mhaigh

Pull down these commits:

hg pull review -r 179069f2757650000640af8bed6693572efb34a7

Comment 47

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

mhaigh already r+'d; over to Margaret for final two reviews.

Comment 48

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/6001/#review4963

Ship It!

Comment 49

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/6003/#review4965

Ship It!

Comment 50

[:Margaret Leibovic]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

https://reviewboard.mozilla.org/r/3437/#review4967

Ship It!

Comment 51

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Antlam mostly confirmed the dialogue over irc (comment 45) so I'm just going to land it.

Comment 52

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=6991efa59daf

Comment 53

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

The share overlay will be getting more publicity in 38 so we'd like to push some polish changes.

Approval Request Comment
[Feature/regressing bug #]: Original implementation

[User impact if declined]: Users will have a less polished share overlay experience than we would like

[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: 
  Low - mostly style updates. Worst case is that we try to access and non-existent View or make a bad cast and crash, while other errors would be having the new styles react in unexpected ways on edge case devices (e.g. small screen, extreme dimensions) and look ugly

[String/UUID change made/needed]: None

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d9a05956ad8
https://hg.mozilla.org/mozilla-central/rev/9a2166dd61dc
https://hg.mozilla.org/mozilla-central/rev/b29f773bd048
https://hg.mozilla.org/mozilla-central/rev/ba4e9c917199
https://hg.mozilla.org/mozilla-central/rev/1c34fab43fa4
https://hg.mozilla.org/mozilla-central/rev/7f0f273c14a7
https://hg.mozilla.org/mozilla-central/rev/d8679845c05e
https://hg.mozilla.org/mozilla-central/rev/9b7ad0bbbdff
https://hg.mozilla.org/mozilla-central/rev/470465f520bf
https://hg.mozilla.org/mozilla-central/rev/3fefbfbc7e51
https://hg.mozilla.org/mozilla-central/rev/cd8b1c84f1ea
https://hg.mozilla.org/mozilla-central/rev/a452fed6c911
https://hg.mozilla.org/mozilla-central/rev/4ce114cbbdb3
https://hg.mozilla.org/mozilla-central/rev/6757510c20c0
https://hg.mozilla.org/mozilla-central/rev/570d7c14c38d
https://hg.mozilla.org/mozilla-central/rev/70cebff5abed
https://hg.mozilla.org/mozilla-central/rev/8c3212c240bc
https://hg.mozilla.org/mozilla-central/rev/4000ac847fc7
https://hg.mozilla.org/mozilla-central/rev/b3901be2155d
https://hg.mozilla.org/mozilla-central/rev/764ccc7fdc30
https://hg.mozilla.org/mozilla-central/rev/6991efa59daf

Comment 55

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/mozilla-central/rev/54b0e2df669c has the wrong bug # in the commit message and is also this bug.

Comment 56

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8582770 [details]
Failure toast

hm, that doesn't feel much like an "error"..

String change? "Sorry! Your tab could not be sent."

Comment 57

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #56)
> String change? "Sorry! Your tab could not be sent."

Spoke on irc, we'll land this in 39.

Comment 58

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

bug 1147535.

Comment 59

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #57)
> (In reply to Anthony Lam (:antlam) from comment #56)
> > String change? "Sorry! Your tab could not be sent."
> 
> Spoke on irc, we'll land this in 39.

bug 1147535.

Comment 60

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Samsung S5 (Android 4.4)
Builds: Firefox for Android 38.0a2 and 39.0a1 (2015-03-26)

Scenarios:
- Set up sync on Aurora, go to Menu -> Add to nightly, the following dialogs are displayed: http://i.imgur.com/pytwDht.png; go to Menu -> Add to nightly-> Send to other devices, choose a device, the following notification is displayed: http://i.imgur.com/jnZpWKy.png

- On Nightly, go to Menu -> Send to other devices, the following dialog is displayed:  http://i.imgur.com/UvGq8u0.png

Comment 61

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unlike the past few bugs, I will not be handling this uplift. NI Sylvestre to make sure it gets done.

Comment 62

[Sylvestre Ledru [:Sylvestre]]

ok, a sheriff will take care of that (no need to n-i me here, we are back on the default workflow)

Comment 63

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The uplift should land with bug 1148041 which I just filed and uploaded a patch - I asked for a review by EOD.

Comment 64

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #63)
> The uplift should land with bug 1148041 which I just filed and uploaded a
> patch - I asked for a review by EOD.

And bug 1148197.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Conflicts on the first patch = I'm leaving this uplift to you.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

This missed the uplift to Aurora. It'll need to go through the Beta approval process.

Comment 67

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

Approval Request Comment 53

Comment 68

[Sylvestre Ledru [:Sylvestre]]

Michael, did you fix the conflict?

Comment 69

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> Michael, did you fix the conflict?

I just finished fixing the issues but I didn't have a chance to test it (nor a chance to rebase the several blocking bugs that go on top - presumably they would be clean rebases).

Comment 70

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

While rebasing, it looks like this actually uses colors from [1] and thus depends on bug 1134484. The simplest solution is just to swipe that simple patch (and not the following patches which replace all the existing colors) so I'm going to graft it on top of my rebase.

[1]: https://hg.mozilla.org/integration/fx-team/rev/e124c25545d2

Comment 71

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And the dropshadow asset from bug 1137921 - I'll have to craft a custom commit for that one.

Comment 72

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I have patches ready to go for this but I discovered bug 1151089 so I'll hold off until that lands and gets aurora/beta approval.

Comment 73

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

Michael, with the other uplifts request, this is starting to be confusing and risky... We are landing too much changes for this feature.
As 38 has already enough features and we are planning to use this release for important communications, I would like to delay this for 39. For now, I am removing my beta approval.

Comment 74

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

A lot of the work I put into 38 deals with new features to the share overlay so I'd like for this bug, a visual refinement to the share overlay, to land so the new features are more appealing to use. I'll summarize which will hopefully simplify the dependency management:

The main work here is a reworking of the styles of the share overlay - re-arranging Views and changing styling. This has a tendency to cause regressions on Android related to different device configurations:
  * bug 1148197 - I revised the styles to properly handle devices that couldn't show the full width of the dialog

We've been making an overall effort to clean up our internal styles and this prompted me to remove the base text styles we've been using all along (why not lean on Android's default styles?). However, because of android fragmentation, the default styles across different devices is different so there is value in creating and maintaining our own base style, thus:
  * bug 1148041 comment 4 - inherited from our base text style (again)
However, the base text style uses an android reference attribute (`?android:attr/...`) which for the share overlay points to the default Android styles which, curiously enough, are private for some elements and make us crash. Why didn't this crash in the main browser Activity? We overrode the default Android styles with the base "Gecko" theme. So:
  * bug 1148041 comment 13 - inherit from the base Gecko theme in the share overlay

I made a mistake when I refactored the styles - I added an animation to the end of the last method called before the share overlay Activity starts, but later added an early return when we run the overlay in internal mode ("Send to other devices") as opposed to external mode ("Add to Firefox"). This meant the animation never got set:
  * bug 1151089 - Always display the entry animation

Finally, the styles I revised referenced some of the new styles we've only recently been incorporating - some of these elements are not available in 38: the standardized color palette, bug 1134484, and the drop shadow from bug 1137921. However, the fix is simple: add in a few hex colors and add a drawable at several different DPI levels respectively - I added commits to fix this locally, but I can post the patches here if you'd like to see.

Comment 75

[Sylvestre Ledru [:Sylvestre]]

I understand that you want to polish as much as possible your work (which is great) but as it is not critical, we could postpone that to 39 or 40, don't you think?

Comment 76

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

Discuss on IRC. Taking it as the current l&f is not good enough. Michael is going to take of all the landing.
I asked for a verify from QE on all these bugs.

Comment 77

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (38 rebase) Add dropshadow assets

Comment 78

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (38 rebase) Add Fennec color palette

Comment 79

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8589620 [details] [diff] [review]
(38 rebase) Add dropshadow assets

Approval Request Comment
[Feature/regressing bug #]: We're rebased this bug but these resources are not available in 38.

[User impact if declined]:
  Users will not see the polished share overlay dialog.

[Describe test coverage new/current, TreeHerder]:
  Local testing

[Risks and why]: 
  Low - adding some assets. Worst case, we use the wrong name and break in compile, but this is already working in 39.

[String/UUID change made/needed]: None

Comment 80

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8589621 [details] [diff] [review]
(38 rebase) Add Fennec color palette

Approval Request Comment
[Feature/regressing bug #]: We're rebased this bug but these resources are not available in 38.

[User impact if declined]:
  Users will not see the polished share overlay dialog.

[Describe test coverage new/current, TreeHerder]:
  Local testing

[Risks and why]: 
  Low - adding some colors. Worst case, we use the wrong name and break in compile, but this is already working in 39.

[String/UUID change made/needed]: None

Comment 81

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=e399294c9df3

Comment 82

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

All patches necessary for this bug in 38 should have landed on 38 (beta) - yay!

Comment 85

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Update ShareOverlay icon padding & assets. r=mhaigh

Comment 86

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Clean up share overlay toast styles. r=mhaigh

Comment 87

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Reset the first item background drawable state onResume. r=mhaigh

Comment 88

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Clean up OverlayDialogButton's initialization. r=mhaigh

Comment 89

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Add drop shadow to overlay share dialog result toast. r=mhaigh

Comment 90

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Update share overlay text colors to match mocks. r=mhaigh

Comment 91

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Rename TextAppearance.ShareOverlay to ShareOverlayTextAppearance. r=mhaigh

Comment 92

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130302 - Move ShareOverlayButton.Text to ShareOverlayTextAppearance.Button. r=mhaigh

Comment 93

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove excess LinearLayout from ShareOverlay. r=mhaigh

Comment 94

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove unused share overlay layout. r=mhaigh

Comment 95

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Clean up style inheritance in share overlay. r=mhaigh

Comment 96

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Update share overlay row pressed color & color names. r=mhaigh

Comment 97

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Review: Remove single use styles in share overlay. r=trivial

Comment 98

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Review: Finish off share overlay nits. r=trivial

Comment 99

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove header container in share overlay & roughly style text. r=mhaigh

Comment 100

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove retry button in share overlay retry toast. r=mhaigh

Comment 101

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove Firefox logo from share overlay. r=mhaigh

Comment 102

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Move share overlay title styles into styles.xml and revise to match mocks. r=mhaigh

Comment 103

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Remove dividers in share overlay. r=mhaigh

Comment 104

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Add @dimen/button_corner_radius and replace corner radius use in code. r=mhaigh

Comment 105

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Round the corners of the first item in the share overlay. r=mhaigh

Comment 106

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1130203 - Set width for share overlay. r=mhaigh

Comment 107

[Mira Lobontiu (Android QA)]

Hi Sylvestre,

Is the qe-verify+ flag still valid?

Thank you!

Comment 108

[Sylvestre Ledru [:Sylvestre]]

As it was 4 years ago, I think this is a bit late ;)

Comment 109

[Mira Lobontiu (Android QA)]

Taking into consideration Comment 108, I will remove the qe-verify+ flag.

Thanks!