840267 - Sync UI using light-holo in setup/JPAKE UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (02/11) - Screenshot

See screenshot.

Comment 1

[Richard Newman [:rnewman]]

Is this just a polish bug?

Is there a trivial fix?

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

If UX is OK with using the light theme here, there's not much to do. This switches us to use standard colors for link text, since we're using a standard background. I'll post a screenshot from my S3 here, but will have to charge some other devices to test on any other themes.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Screenshot

I don't think anyone can steal that code for evil.... can they?

Comment 4

[Richard Newman [:rnewman]]

J-PAKE codes expire after five minutes.

Comment 5

[Ian Barlow (:ibarlow)]

If possible, I'd like to use a more Holo style title bar and Cancel button area here. Also let's reverse the JPAKE fields/numbers so it is dark numbers on a light background.

Comment 6

[Richard Newman [:rnewman]]

Wes, are you going to run with this?

Comment 7

[Wesley Johnston (:wesj)]

Sure

Comment 8

[Richard Newman [:rnewman]]

Thanks!

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This is me trying to steal from the alert dialog style as much as I can. I had to do some trickery to get lines around the buttonBox.

http://androidxref.com/4.0.4/xref/frameworks/base/core/res/res/layout/alert_dialog_holo.xml#71

I'll post some screenshots from a ICS and Gingerbread phone.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Screenshots

Comment 11

[Ian Barlow (:ibarlow)]

ship it!

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 713725 [details] [diff] [review]
Patch v1

Review of attachment 713725 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just trailing whitespace. I will backport this to upstream.

Thanks Wes!

::: mobile/android/base/resources/layout/sync_account.xml
@@ +62,4 @@
>  
> +    <LinearLayout
> +      style="@style/SyncBottom" >
> +  

Trailing whitespace in a couple of places.

Comment 13

[Richard Newman [:rnewman]]

Backport pull request:

https://github.com/mozilla-services/android-sync/pull/293

Wes, I think you might have forgotten to `hg add` the v11 style file. Take a look?

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sorry. Forgot to hg add it.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 714041 [details] [diff] [review]
Patch v2

Review of attachment 714041 [details] [diff] [review]:
-----------------------------------------------------------------

If you've tested it, looks good to me!

Comment 16

[Richard Newman [:rnewman]]

Merged in GitHub. Waiting for inbound to open so wesj can land (or I can if he disappears).

Comment 17

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f46e28183c02

Comment 18

[Gregory Szorc [:gps]]

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

Comment 19

[Richard Newman [:rnewman]]

I landed a follow-up to this to fix some whitespace in the landed patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/12a80c7bb01a

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12a80c7bb01a

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 714041 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 832321
User impact if declined: Sync setup is hard to read
Testing completed (on m-c, etc.): Has been on mc for a few weeks. No regressions
Risk to taking this patch (and alternatives if risky): Low risk. Mostly themeing
String or UUID changes made by this patch: None.

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 714041 [details] [diff] [review]
Patch v2

low risk UI polish helping existing sync set-up on android.Approving for uplift

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bc6bb37e6a5