1132986 - Use light-theme default system dialog in "Send tab to device" dialog

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We currently use the dark one and it's out of place.

Comment 1

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

We should keep it the same as whatever the other dialogs look like for the user

Comment 2

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

Attachment: MozReview Request: bz://1132986/mcomella

/r/4333 - Bug 1132986 - Display a Gecko-themed dialog when sending tabs to device. r=liuche

Pull down this commit:

hg pull review -r abb79f643bc4ed5b1035e813a5ed5e83a3180971

Comment 3

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

Attachment: Screenshot of fix

Anthony, this is what you're looking for, right? It looks like the long-press context menus.

Comment 4

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

Comment on attachment 8569562 [details]
Screenshot of fix

I don't remember "looking for" this.. but I think this still applies to "Add to Firefox" from outside the app right?

Comment 5

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

(In reply to Anthony Lam (:antlam) from comment #4)
> I don't remember "looking for" this.. but I think this still applies to "Add
> to Firefox" from outside the app right?

Just tested - yep.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8569561 [details]
MozReview Request: bz://1132986/mcomella

https://reviewboard.mozilla.org/r/4331/#review3631

Ship It!

Comment 7

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

I assume this affects 37 and 38, but I have not tested myself.

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/be451dd958bf

Comment 9

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

Comment on attachment 8569561 [details]
MozReview Request: bz://1132986/mcomella

Note that this is already shipping in 36.

Approval Request Comment
[Feature/regressing bug #]: Share overlay

[User impact if declined]:
  Users with more than 3 synced devices (granted, a minority), when they open the share overlay (i.e. click "Add to Firefox") and then click to share to a device, will be presented with a black menu, rather than white. The key detail here is that our app is themed white so this is an inconsistency in styles and looks unpolished.

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  Low - we change the theme of the dialog to an existing theme. We also change one of the attributes in the theme to hide the background window, which I'm a little more nervous of the side effects of, but I think it's fairly low risk.

[String/UUID change made/needed]:

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Michael Comella (:mcomella) from comment #7)
> I assume this affects 37 and 38, but I have not tested myself.

Can you please confirm that this is actually an issue on these channels before we uplift the code? Or, if you've confirmed that both 36 and 39 are affected, I think we can safely assume that 37 and 38 are affected too.

(In reply to Michael Comella (:mcomella) from comment #9)
> Note that this is already shipping in 36.

I think you mean that we shipped this bug in 36. Is that right?

> [User impact if declined]:
>   Users with more than 3 synced devices (granted, a minority), when they
> open the share overlay (i.e. click "Add to Firefox") and then click to share
> to a device, will be presented with a black menu, rather than white. The key
> detail here is that our app is themed white so this is an inconsistency in
> styles and looks unpolished.

I think this is an edge case that we can live with but, as the fix is small, I'm happy to take this in Beta 2 if it hits m-c and can be verified before we gtb on Monday afternoon ET.

> [Risks and why]: 
>   Low - we change the theme of the dialog to an existing theme. We also
> change one of the attributes in the theme to hide the background window,
> which I'm a little more nervous of the side effects of, but I think it's
> fairly low risk.

Can you please elaborate on what side effects are possible as a result of this change?

Comment 11

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #10)
> Or, if you've confirmed that both 36 and 39 are
> affected, I think we can safely assume that 37 and 38 are affected too.

I have and I agree - I tried to be explicit about my actions but I'll try to be more explicit about my assumptions next time too. :P

> > Note that this is already shipping in 36.
> 
> I think you mean that we shipped this bug in 36. Is that right?

Yep.

> > We also
> > change one of the attributes in the theme to hide the background window,
> > which I'm a little more nervous of the side effects of, but I think it's
> > fairly low risk.
> 
> Can you please elaborate on what side effects are possible as a result of
> this change?

I am not too aware of the attributes used by Window in Android applications (e.g. I was surprised to see both a non-transparent window color and non-fullscreen view specified by default when I initially wrote this patch). The android:windowBackground [1] attribute seems straight-forward enough (i.e. the color of the background window and if we're setting it transparent as we do here, it will probably just go away and not bother us), but I'm concerned about attributes I might need to set for edge case uses (e.g. distinction of opening this through the Android launcher vs. another application). However, the scope of where this code is accessed is limited (just via other applications), so I don't see many edge cases coming up.

Again, just trying to be (overly) explicit! :)

[1]: https://developer.android.com/reference/android/R.attr.html#windowBackground

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Michael Comella (:mcomella) from comment #11)
> > Can you please elaborate on what side effects are possible as a result of
> > this change?
> 
> I am not too aware of the attributes used by Window in Android applications
> (e.g. I was surprised to see both a non-transparent window color and
> non-fullscreen view specified by default when I initially wrote this patch).
> The android:windowBackground [1] attribute seems straight-forward enough
> (i.e. the color of the background window and if we're setting it transparent
> as we do here, it will probably just go away and not bother us), but I'm
> concerned about attributes I might need to set for edge case uses (e.g.
> distinction of opening this through the Android launcher vs. another
> application). However, the scope of where this code is accessed is limited
> (just via other applications), so I don't see many edge cases coming up.
> 
> Again, just trying to be (overly) explicit! :)

I appreciate that. This information should be useful for QA exploratory testing.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/be451dd958bf

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8569561 [details]
MozReview Request: bz://1132986/mcomella

Taking this for aurora,  but leaving this to Lawrence to make the call, since in comment 10 we were looking to uplift it for beta 2 and it's slightly too late for that.

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-03-04-15-09-53.png

Tested using:
Device:LG Nexus 4 (Android 4.4)
Build: Firefox for Android 39.0a1 (2015-03-04)
Open a page and go to Menu -> Share -> Add to Nightly -> Send to other devices => a white dialog "Select device" will be displayed

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8569561 [details]
MozReview Request: bz://1132986/mcomella

The fix has been verified on Nightly. I think we're fine to take this in Beta 3. Beta+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9aa3163b6d36

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/003b419b893f

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Tested using:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 38.0a2 (2015-03-06)
Opening a page and choosing to "Add to Aurora" and "Send to other devices" , will display a white dialog "Select device".

Comment 20

[Teodora Vermesan (:TeoVermesan)]

A light-theme dialog is displayed when choosing to send a tab to other devices, so:
Verified fixed using:
Device: Sony Xperia Z2 (Android 4.4.4) 
Build: Firefox for Android 37 Beta 4

Comment 22

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

Attachment: MozReview Request: Bug 1132986 - Display a Gecko-themed dialog when sending tabs to device. r=liuche