1412518 - Low contrast/badly readable text in time and date pickers with Photon colour scheme

Status: VERIFIED FIXED

(Firefox for Android :: Theme and Visual Design, defect)

Description

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]:
The time picker uses black for the currently active input (hours or minutes) and some sort of medium grey for the inactive input.
Previously this was displayed against an orange background, but the new colour scheme uses blue here, which makes the grey text rather badly readable.

Comment 1

[Jan Henning [:JanH]]

Attachment: Screenshot_20171028-144740.png

Comment 2

[Carol Huang [:Carol]]

Attachment: Time Picker.png

I'll leave the visual change(spec) here. Let's fix it once the engineer have time.
Thanks!

Comment 3

[Marcia Knous [:marcia - needinfo? me]]

Comment 2 makes me believe maybe this doesn't need to be tracked for 57 and can wait until 58? We are already close to the first RC.

Comment 4

[Jan Henning [:JanH]]

Not ideal, but given the time scale no objection.

Comment 5

[Marcia Knous [:marcia - needinfo? me]]

[Tracking Requested - why for this release]:

Won't fix for 57. Let's get this fixed for 58.

Comment 6

[Julien Cristau [:jcristau]]

This didn't happen for 58, and I'm guessing won't for 59 either...

Comment 7

[Jan Henning [:JanH]]

Because of bug 1318046 now the date picker, too...

Comment 8

[Petru-Mugurel Lingurar[:petru]]

Attachment: Bug 1412518 - Photon themed TimePicker;

On Api >=21 the Material TimePicker will have applied a Photon style.
Below Api 21 TimePicker will still be using spinners.

Review commit: https://reviewboard.mozilla.org/r/259378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259378/

Comment 9

[Petru-Mugurel Lingurar[:petru]]

Attachment: Bug 1412518 - Photon themed DatePicker;

On Api >=21 the Material DatePicker will have applied a Photon style.
Below Api 21 DatePicker will still be using spinners.

Review commit: https://reviewboard.mozilla.org/r/259380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259380/

Comment 10

[Petru-Mugurel Lingurar[:petru]]

I've applied the colors from Carol in new styles for the DatePicker and TimePicker.
The new stylized pickers will be available on API >= 21. Below this for selecting date/time spinners are being used.

How they look:
Photon TimePicker - https://drive.google.com/file/d/1sXqf_aDeqxIvQTG1gbh_-mlJf4qwtAJX/view?usp=sharing
Photon DatePicker - https://drive.google.com/file/d/1deGkrYNkE0xnN1PWNqS0Ie0EQG5U_eIS/view?usp=sharing

NI-ing Bram to confirm if the style Carol proposed is still up-to-date and if the changes look fine.

Comment 11

[Bram Pitoyo [:bram]]

Hi Petru, Carol’s design looks really good. However, do you know if there’s a reason behind setting the background colour of the date/time selection grey? It just looks different from the white background of the modal window.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8994841 [details]
Bug 1412518 - Photon themed TimePicker;

https://reviewboard.mozilla.org/r/259378/#review266564

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8994842 [details]
Bug 1412518 - Photon themed DatePicker;

https://reviewboard.mozilla.org/r/259380/#review266566

Comment 14

[Carol Huang [:Carol]]

(In reply to Bram Pitoyo [:bram] from comment #11)
> Hi Petru, Carol’s design looks really good. However, do you know if there’s
> a reason behind setting the background colour of the date/time selection
> grey? It just looks different from the white background of the modal window.

Hey Bram,
Are you talking about the grey circle background behind the date/time selection?
At the time, I'm only helping out on tweaking the color because we applied the photon Blue which makes the text low contrast.
I think the picker might be the original android component design like the one shows here: http://www.zoftino.com/android-timepicker-example

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85bdb30a33ed
Photon themed TimePicker; r=jchen
https://hg.mozilla.org/integration/autoland/rev/313ec19aa619
Photon themed DatePicker; r=jchen

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a57376bf59077b6d64bcca8ba704ca1b6324383e -d 313ec19aa619: rebasing 474682:a57376bf5907 "Bug 1412518 - Photon themed TimePicker; r=jchen"
merging mobile/android/app/src/main/res/values/themes.xml
merging mobile/android/app/src/photon/res/values/styles.xml
warning: conflicts while merging mobile/android/app/src/main/res/values/themes.xml! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/app/src/photon/res/values/styles.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/85bdb30a33ed
https://hg.mozilla.org/mozilla-central/rev/313ec19aa619

Comment 18

[Oana Horvath]

Verified as fixed on Nightly 63. 
Devices verified:
Google Pixel (Android P Beta)
HTC Desire 820 (Android 6.0.1)
Samsung Galaxy Tab 3 (Android 8.0)

Comment 19

[Bram Pitoyo [:bram]]

Hi Jan,

If we have some resource available, I want to let you know that we also have the opportunity to slightly refresh our design to make it even more in-line with the Photon Design System.

See examples I’m going to post below.

Comment 20

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Comment 21

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Comment 22

[Petru-Mugurel Lingurar[:petru]]

Comment on attachment 8994841 [details]
Bug 1412518 - Photon themed TimePicker;

Approval Request Comment
[Feature/Bug causing the regression]: Material TimePicker shows hard to read content.
[User impact if declined]: Material TimePicker shows hard to read content. Used theme is not aligned with the Photon designed.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: --
[List of other uplifts needed for the feature/fix]: --
[Is the change risky?]: No
[Why is the change risky/not risky?]: Small change, just a style change; Has been QE verified and in the time being in Nightly no issues have been reported
[String changes made/needed]: --

Comment 23

[Petru-Mugurel Lingurar[:petru]]

Comment on attachment 8994842 [details]
Bug 1412518 - Photon themed DatePicker;

Approval Request Comment
[Feature/Bug causing the regression]: Material DatePicker shows hard to read content.
[User impact if declined]: Material DatePicker shows hard to read content. Used theme is not aligned with the Photon designed.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: --
[List of other uplifts needed for the feature/fix]: --
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just a style change; Has been QE verified and in the time being in Nightly no issues have been reported.
[String changes made/needed]: --

Comment 24

[Jan Henning [:JanH]]

(In reply to Bram Pitoyo [:bram] from comment #19)
> Hi Jan,
> 
> If we have some resource available, I want to let you know that we also have
> the opportunity to slightly refresh our design to make it even more in-line
> with the Photon Design System.
> 
> See examples I’m going to post below.

Feel free to file a follow-up bug for that if you wish.

Comment 25

[Liz Henry (:lizzard)]

Comment on attachment 8994841 [details]
Bug 1412518 - Photon themed TimePicker;

Low-risk style fix for a contrast issue. And we still have a couple of weeks before the RC build. Let's uplift for beta 16.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/368a3770259c
https://hg.mozilla.org/releases/mozilla-beta/rev/161450968865

Comment 27

[Ioana Chiorean]

Can you please check this, Oana?

Comment 28

[Oana Horvath]

Verified on Beta 62.0b17. Logged Bug 1483165, caused by this change.