Closed Bug 1130203 Opened 9 years ago Closed 9 years ago

Implement v2 share overlay mock

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(32 files, 6 obsolete files)

1.21 MB, image/png
Details
631.55 KB, image/png
Details
24.37 KB, application/zip
Details
126.21 KB, image/png
antlam
: feedback+
Details
37.00 KB, application/zip
Details
4.00 KB, application/zip
Details
148.35 KB, image/png
antlam
: feedback+
Details
389.41 KB, image/png
antlam
: feedback-
Details
4.79 KB, patch
Details | Diff | Splinter Review
2.10 KB, patch
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
mcomella
: review+
Details
Assignee: nobody → michael.l.comella
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.
Flags: needinfo?(alam)
/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
Attachment #8560230 - Flags: review?(mhaigh)
Note: changes from comment 2 are not yet final.
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...
Flags: needinfo?(alam)
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?
Attachment #8560230 - Flags: review?(mhaigh) → review+
(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.
Flags: needinfo?(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.
Flags: needinfo?(mhaigh)
Attached image WIP (obsolete) —
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.
Attachment #8570282 - Flags: feedback?(alam)
Anthony, also how wide should the dialog be?
And what color should the pressed & focused states be?
(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 on attachment 8570282 [details]
WIP

Will attach specs :)
Attachment #8570282 - Flags: feedback?(alam) → feedback-
Attached file shareoverlay2_icons.zip (obsolete) —
Also, found these (which I think are also in the other bugs)
Attached file icon_greencheck.zip
Adding check (forgot!)
Summary: Implement latest share overlay mock → Implement v2 share overlay mock
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
Attachment #8560230 - Flags: review+ → review?(mhaigh)
Still TODO:
  * (followup?) Clean up styles
  * (followup?) Add back drop shadow (liuche mentioned she was doing a bug on this)
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?
Flags: needinfo?(alam)
TODOs mentioned above this comment - what do you think, Anthony?
Attachment #8570826 - Flags: feedback?(alam)
Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

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

Ship It!
Attachment #8560230 - Flags: review?(mhaigh) → review+
Comment on attachment 8570826 [details]
Screenshot (post commits)

Looking good! We'll also need to update that "Add to RL" icon there
Flags: needinfo?(alam)
Attachment #8570826 - Flags: feedback?(alam) → feedback+
^Oh, text should be #363B40 (text and tabs tray grey) right? looks a bit light there.
Included the "Add to Reading List" icon and updated colors. Will also mark obsolete all icons in bug 1059554
Attachment #8570748 - Attachment is obsolete: true
(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.
Attachment #8560230 - Flags: review+ → review?(mhaigh)
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
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
QA Contact: flaviu.cos
Already added to Reading List icon plz!
Flags: needinfo?(alam)
Attached file icon_RLed.zip
Here ya go
Flags: needinfo?(alam)
Attached image Success toast (obsolete) —
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.
Attachment #8580380 - Flags: feedback?(alam)
Attached image Toast (retry) (obsolete) —
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 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 33 should be final pending antlam approval (until we revise the toast at a later point).
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.
Attachment #8580380 - Flags: feedback?(alam) → feedback-
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 :)
Flags: needinfo?(michael.l.comella)
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!
Attachment #8560230 - Flags: review?(mhaigh) → review+
(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.
Flags: needinfo?(alam)
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.
(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.
(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?
Flags: needinfo?(alam)
(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.
Flags: needinfo?(michael.l.comella)
Attachment #8580380 - Attachment is obsolete: true
Attachment #8582140 - Flags: feedback?(alam)
Comment on attachment 8582140 [details]
Success toast w/ dropshadow

Niiiiice!
Attachment #8582140 - Flags: feedback?(alam) → feedback+
Attached image Failure toast
Pretty simple.
Attachment #8580383 - Attachment is obsolete: true
Attachment #8580405 - Attachment is obsolete: true
Attachment #8582770 - Flags: feedback?(alam)
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
Attachment #8560230 - Flags: review?(mhaigh)
Attachment #8560230 - Flags: review?(margaret.leibovic)
Attachment #8560230 - Flags: review+
Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

mhaigh already r+'d; over to Margaret for final two reviews.
Attachment #8560230 - Flags: review?(mhaigh) → review+
Attachment #8560230 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

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

Ship It!
Antlam mostly confirmed the dialogue over irc (comment 45) so I'm just going to land it.
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
Attachment #8560230 - Flags: approval-mozilla-aurora?
QA Contact: flaviu.cos → teodora.vermesan
https://hg.mozilla.org/mozilla-central/rev/54b0e2df669c has the wrong bug # in the commit message and is also this bug.
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."
Flags: needinfo?(michael.l.comella)
Attachment #8582770 - Flags: feedback?(alam) → feedback-
(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.
Flags: needinfo?(michael.l.comella)
(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.
Attachment #8560230 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Unlike the past few bugs, I will not be handling this uplift. NI Sylvestre to make sure it gets done.
Flags: needinfo?(sledru)
ok, a sheriff will take care of that (no need to n-i me here, we are back on the default workflow)
Flags: needinfo?(sledru)
The uplift should land with bug 1148041 which I just filed and uploaded a patch - I asked for a review by EOD.
(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.
Conflicts on the first patch = I'm leaving this uplift to you.
Flags: needinfo?(michael.l.comella)
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.
Attachment #8560230 - Flags: approval-mozilla-aurora+
Comment on attachment 8560230 [details]
MozReview Request: bz://1130203/mcomella

Approval Request Comment 53
Attachment #8560230 - Flags: approval-mozilla-beta?
Attachment #8560230 - Flags: approval-mozilla-aurora?
Michael, did you fix the conflict?
(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).
Flags: needinfo?(michael.l.comella)
Attachment #8560230 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Depends on: 1134484
And the dropshadow asset from bug 1137921 - I'll have to craft a custom commit for that one.
Depends on: 1137921
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 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.
Attachment #8560230 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
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.
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?
Flags: qe-verify+
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.
Attachment #8560230 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Attachment #8589620 - Flags: approval-mozilla-beta?
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
Attachment #8589621 - Flags: approval-mozilla-beta?
Attachment #8589620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8589621 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
All patches necessary for this bug in 38 should have landed on 38 (beta) - yay!
Depends on: 1154489
Attachment #8560230 - Attachment is obsolete: true
Attachment #8619353 - Flags: review+
Attachment #8619354 - Flags: review+
Attachment #8619355 - Flags: review+
Attachment #8619356 - Flags: review+
Attachment #8619357 - Flags: review+
Attachment #8619358 - Flags: review+
Attachment #8619359 - Flags: review+
Attachment #8619360 - Flags: review+
Attachment #8619361 - Flags: review+
Attachment #8619362 - Flags: review+
Attachment #8619363 - Flags: review+
Attachment #8619364 - Flags: review+
Attachment #8619365 - Flags: review+
Attachment #8619366 - Flags: review+
Attachment #8619367 - Flags: review+
Attachment #8619368 - Flags: review+
Attachment #8619369 - Flags: review+
Attachment #8619370 - Flags: review+
Attachment #8619371 - Flags: review+
Attachment #8619372 - Flags: review+
Attachment #8619373 - Flags: review+
Attachment #8619374 - Flags: review+
Hi Sylvestre,

Is the qe-verify+ flag still valid?

Thank you!
Flags: needinfo?(sledru)
As it was 4 years ago, I think this is a bit late ;)
Flags: needinfo?(sledru)
Taking into consideration Comment 108, I will remove the qe-verify+ flag.

Thanks!
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.