Closed Bug 1049824 Opened 5 years ago Closed 5 years ago

[Settings] Add color filter settings in accessibility panel

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S4 (12sep)
tracking-b2g backlog
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Keywords: access)

Attachments

(3 files, 1 obsolete file)

Once we have support for this from graphics, we need to add a settings panel for color filter settings in the accessibility section.
No longer depends on: 1016539
To summarize, we'd need:
Invert colors (checkbox, default off)
Use grayscale (checkbox, default off)
Enhance Contrast (continuous slider, range 0 to 1, default 0 - probably don't need to show the actual values).

If this was an image enhancement feature, rather than accessibility, we would have contrast go from -1 to 1, where the negative values would reduce contrast, but I'm pretty sure that isn't needed.
(In reply to Milan Sreckovic [:milan] from comment #1)
> To summarize, we'd need:
> Invert colors (checkbox, default off)
> Use grayscale (checkbox, default off)
> Enhance Contrast (continuous slider, range 0 to 1, default 0 - probably
> don't need to show the actual values).
> 
> If this was an image enhancement feature, rather than accessibility, we
> would have contrast go from -1 to 1, where the negative values would reduce
> contrast, but I'm pretty sure that isn't needed.

Low contrast has some accessibility value as well, it can be a preference for some reading disabilities like dyslexia. So, we should probably consider too.
That's good info.  I was guessing based on what OS X was doing, but you have better info.

In that case, the default for contrast would probably be "in the middle", and I imagine we'd want some kind of a reset or "snap to the middle" behavior.  There is cost associated with contrast not being the default, though it's mostly on the memory side.

I want to make sure this doesn't get missed on the Gaia side, so adding David into the conversation, to see how we start a conversation as to whether this can be done in 2.1 and who can do it, etc.
Flags: needinfo?(dscravaglieri)
Good outline for UX specs. I would add two additional ideas:

1. A global quick gesture for toggling filters. This is useful for users who encounter white on black text and don't want it inverted, or if they encounter images and want to see them unfiltered. Also, for folks who have borderline disabilities, it is nice to be able to quickly enhance the view for a few moments to parse something that is otherwise challenging.

2. Perhaps the panel could have some "preview visuals" so the user could see the effect in action. Something like a few lines of text with different colors and sizes, and maybe an image. Not a must, but could be nice.
Nice.  Stephany, could you find us a "UX contact" for this bug to officially bless what's evolving here, to make sure we don't go astray from some overall UX design?  I don't think we need a lot, just somebody to keep us honest.
Flags: needinfo?(swilkes)
I'm actually going to flag three folks here, Milan. :) Przemek for the visual check, Harly and Juwei on Settings and the rest.
Flags: needinfo?(swilkes)
Flags: needinfo?(pabratowski)
Flags: needinfo?(jhuang)
Flags: needinfo?(hhsu)
Hi Stephany,
I think you meant Jenny not Juwei, as Jenny is the UX in charge of Settings. Also flagging Helen as she is the visual designer for Settings.

Hi Eitan & Milan,
It seems that the color filtering settings will be within the accessibility panel in system settings, which I think it make sense. The only concern I have is the global quick gesture for toggling filter, if you guys have any proposal, please let us know so we could be certain that it wouldn't conflict with other system gestures. Thanks
Flags: needinfo?(jhuang)
Flags: needinfo?(jelee)
Flags: needinfo?(hhuang)
Flags: needinfo?(hhsu)
Hello, let me know if you need any UX support for accessibility settings, I'd be happy to take a look at your proposed spec when it's ready. Thanks!
Flags: needinfo?(jelee)
I don't have much to add, I feel it's being covered now :)
Flags: needinfo?(pabratowski)
Hi, I'd like to support if there is any visual needs. Please feel free to let me know. Thanks!
Flags: needinfo?(hhuang)
Tagging this as 2.1 to check with Gaia on whether this will make it in on time or not.
feature-b2g: --- → 2.1
Hi Tim, seems like Milan ni the wrong person? If so, please help me to remove the feature-b2g tag because I don't think there's resource for this before 2.1 FL. Thanks.
Flags: needinfo?(timdream)
We could definitely help but not in feature-b2g: 2.1 scope.
blocking-b2g: --- → backlog
feature-b2g: 2.1 → ---
Flags: needinfo?(timdream)
Flags: needinfo?(dscravaglieri)
Actually, I think the work can't start w/o any UX spec from Jenny.

Jenny, could you produce a spec containing what the switch should look like in accessibility panel? do we have that panel?
Flags: needinfo?(jelee)
We have existing common controls and a preview pattern that would probably make sense here. There are also similarities with camera and backgrounds that could extend here.

Jenny, if you can do a proposal, that would be great, and the Frameworks team would love to review it in one of Wednesday meetings, too. Thanks!
Attached file [Settings] Accessibility copy v1.0.pdf (obsolete) —
Hi Eitan, per discussion in mail, please take a look at the initial spec draft, let me know if there's anything needs modification. About the preview feature, I think it'd be better if we simply apply the change right after user turns it on, this way we don't have to make user go through the "apply/cancel" step. Thanks!
Flags: needinfo?(jelee) → needinfo?(eitan)
(In reply to Jenny Lee from comment #16)
> Created attachment 8477254 [details]
> [Settings] Accessibility copy v1.0.pdf
> 
> Hi Eitan, per discussion in mail, please take a look at the initial spec
> draft, let me know if there's anything needs modification. About the preview
> feature, I think it'd be better if we simply apply the change right after
> user turns it on, this way we don't have to make user go through the
> "apply/cancel" step. Thanks!

Hey Jenny,

I like your simple approach. Sounds good. I am wondering if the screen reader should live in its own section. Right now it only has a few sliders, but I could see us adding other options in the future.

So my suggestion is to have an item at the bottom called "Screen Reader" with a description of "Off" in small type below it. Tapping it goes into another section that is dedicated to a screen reader with the toggle switch on top. Very similar to bluetooth and wifi.

What do you think?
Flags: needinfo?(eitan)
The preferences for these are layers.effect.grayscale (true/false, false default), layers.effect.invert (true/false, false default) and layers.effect.contrast (float, 0 default, >0 is the enhanced contrast, <0 is reduced contrast, with 1 giving you twice the contrast and -1 making everything middle gray.)

Setting any of these is not free, memory and performance, so if we are doing a slider for the contrast, we have to make sure we snap to 0 value so that we don't end up with a "close to 0, but not quite" where the user doesn't see any difference, but we pay the price.

For the contrast, I'm not sure if we want the reduction to go all the way to -1 (mid gray), and I'm not sure if the enhancement should stop at 1 or go even higher, but that should be easy enough to test.
(In reply to Milan Sreckovic [:milan] from comment #18)
> The preferences for these are layers.effect.grayscale (true/false, false
> default), layers.effect.invert (true/false, false default) and
> layers.effect.contrast (float, 0 default, >0 is the enhanced contrast, <0 is
> reduced contrast, with 1 giving you twice the contrast and -1 making
> everything middle gray.)
> 
> Setting any of these is not free, memory and performance, so if we are doing
> a slider for the contrast, we have to make sure we snap to 0 value so that
> we don't end up with a "close to 0, but not quite" where the user doesn't
> see any difference, but we pay the price.

Don't know how a slider could snap while allowing subtle changes near the 0 range. Maybe a checkbox that enables/disables the contrast slider could be a possibility as well.
Just checking on the "backlog" flag here, Eitan. Should we mark this as feature-b2g:2.2?
(In reply to Stephany Wilkes from comment #20)
> Just checking on the "backlog" flag here, Eitan. Should we mark this as
> feature-b2g:2.2?

I think we could land this early next week, and have this in 2.1. Wouldn't that be cool?
Yes, but here's the thing: without that flag, no one in QA, release, and so on knows that it was supposed to be there; whether it should or should not block 2.1 or other landings; etc. :) If it was on the agreed feature list for 2.1 and covered during those sprints, and/or can block, the flag should be re-set to 2.1 (which it had earlier). Otherwise it's not going to come up in queries, etc. for 2.1 expected features.
This is on the 2.1 list, and should be tagged as 2.1 or somebody should be informed if we're dropping it for 2.1.
Thanks, Milan. Flagging you and/or David to please make sure *every single accessibility bug* that was agreed for 2.1 has the feature-b2g:2.1 flag set (not just this bug, which I'm setting now). It's the only way for the other teams to understand what is and is not supposed to be in. Thanks!
feature-b2g: --- → 2.1
Flags: needinfo?(milan)
Flags: needinfo?(dbolter)
Passing to Eitan. Eitan can you also make sure to flag the other a11y work with the 'feature-b2g:2.1' tracking as appropriate per Stephany?
Flags: needinfo?(dbolter)
Flags: needinfo?(eitan)
(In reply to Stephany Wilkes from comment #24)
> Thanks, Milan. Flagging you and/or David to please make sure *every single
> accessibility bug* that was agreed for 2.1 has the feature-b2g:2.1 flag set
> (not just this bug, which I'm setting now). It's the only way for the other
> teams to understand what is and is not supposed to be in. Thanks!

Yes, I've done that for the graphics bugs, but I shouldn't be the one to do it for the Gaia side - it should be whoever commits to doing it.  This is why David was NI on this back in comment 3.

Candice, this is why I tagged the 2.1 feature in line 94 in your document as Low confidence for FL landing.  This was noted a few times as "Gaia is not planning on doing the UI", but I'm not sure how far it got.
Flags: needinfo?(milan) → needinfo?(cserran)
Here is a work in progress, based on Jenny's spec. It works with mozilla-central now, and will probably stop working once the preferences are renamed and the contrast patch is committed.

The screen reader settings are hidden by default and can be shown via a flag in the developer menu, similar to how the whole accessibility panel was hidden earlier.

I have some UX suggestions:
The entire color filter section should be toggable, like wifi or bluetooth, this will answer two goals:
1. Like Milan said, the user pays a performance price even if they have the contrast set to something subtle by mistake. So having a on/off switch for the filters as a whole would minimize that risk.

2. This will allow us in the future to have a global quick toggle where the user could switch between filtered and unfiltered mode. This could either be a gesture, or maybe a button in the utility tray? This could be a followup feature.
Gaia didn't commit doing this in 2.1. But since Eitan already has WIP, Settings module owners can help to review the patch once it's done. Put assignee as Eitan then.
Assignee: nobody → eitan
Hey Jenny.

I implemented your spec. I am wondering what you think of my two suggestions above:
1. Have the screen reader in its own sub-section, as I detailed in comment #17.
2. Have a switch to enable/disable color filters, as I detailed in comment #27.
Flags: needinfo?(eitan) → needinfo?(jelee)
Hi Eitan, let's take the discussion offline before a second version ca be released ;)
Flags: needinfo?(jelee)
We should really have all the feature-b2g bugs filed on all components during planning. It might not happen this time, but it's possible you might not be able to get reviewer's attention if they weren't expect your patch is coming!
Status: NEW → ASSIGNED
Flags: needinfo?(cserran)
Depends on: 1059608
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

I updated the pull request to Jenny's latest spec. These changes touch both system and settings modules, along with new Python UI tests. In order to avoid last minute surprise reviews, I'll flag potential reviewers for preliminary feedback. Things might change before final review if Jenny has another spec.
Attachment #8477644 - Flags: feedback?(zcampbell)
Attachment #8477644 - Flags: feedback?(ejchen)
Attachment #8477644 - Flags: feedback?(alive)
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

I didn't run it (as it was f? only) but it looks good and thanks for following the prevailing test code writing style.

Just one addition that if you want to run this on TBPL then add it into tbpl-manifest.ini aswell (although we are trying to obsolete this in favour of the normal manifest)
Attachment #8477644 - Flags: feedback?(zcampbell) → feedback+
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

Pull request is rebased on gaia-header changes, based on Jenny's latest spec, updated tests. I think this is ready for official review.
Attachment #8477644 - Flags: review?(zcampbell)
Attachment #8477644 - Flags: review?(ejchen)
Attachment #8477644 - Flags: review?(alive)
Attachment #8477644 - Flags: feedback?(ejchen)
Attachment #8477644 - Flags: feedback?(alive)
Hi Eitan,

in Settings app, we are going to use requireJS to make panels more modular and easier to maintain. And after checking your code, it seems these three panels are simple enough to use this pattern. Can you take other panels for example and write them in the same way ?

I'll give it a further check after it's done. Thanks :)
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

system app part looks fine, though I have poor background knowledge about this feature.

It would be good if you explain more.
Attachment #8477644 - Flags: review?(alive) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #36)
> Comment on attachment 8477644 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224
> 
> system app part looks fine, though I have poor background knowledge about
> this feature.
> 
> It would be good if you explain more.

A user can tweak the color settings for their device: invert contrast and grayscale. The user may want to disable all filters, and the re-enable the exact settings again. So we can't write the settings directly to layers.effects.{contrast,invert,grayscale}. Instead we have accessibility settings of accessibility.colors.{contrast,invert,grayscale}, in addition to an 'enable' key.

If any accessibility color settings are changed, they will only be copied to the layers setting it the 'enable' setting is true.

If the enable setting is toggled, we either copy all the accessibility settings to layers settings, or set them all to default, for a value of true/false respectively.

This needs to be done outside the settings app because in the future there may be more ways to toggle this, and we want it to take affect immediately. Hope that helps!
EJ, I like the new settings JS setup! I just updated the PR to use it.
Flags: needinfo?(ejchen)
Nice !! I'll give it a check later, thanks Eitan :)
Flags: needinfo?(ejchen)
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

Thanks Eitan, I think Settings part is almost done !

Please check my last few nits on Github ! Thanks :)
Attachment #8477644 - Flags: review?(ejchen)
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

Nits: addressed.
Attachment #8477644 - Flags: review?(ejchen)
@Eitan,

you didn't understand the problem about cancel button I mentioned on Github, please check the screencast here : https://mega.co.nz/#!YwZDVbzB!-87F6cyS1D4nbEjJ2wjVnFnHPQQ4svG0ULSOkC7lgGM
And also, I noticed ScreenReader sometimes is not able to turn off, does this happen there @Eitan ?
And I have one more question, if by default we have panel for accessibility, should we still use the key in developer panel to show/hide it ? Because I think there is no strong reason for now to use that key. Any idea ?
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

Left comments on Github and please fix the problems I mentioned above. 

Thanks @Eitan !
Attachment #8477644 - Flags: review?(ejchen)
Removing feature 2.1 tag since this is not likely to land before 2.1 FL. If necessary, please request uplift when it's done.
feature-b2g: 2.1 → ---
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

I ran these tests locally against the flame. I saw the same error by which this test is failing on TBPL, it needs ot be addressed before the merge.

Aside from that they work fine.
Attachment #8477644 - Flags: review?(zcampbell) → review-
(In reply to EJ Chen [:eragonj][:小龍哥] from comment #42)
> @Eitan,
> 
> you didn't understand the problem about cancel button I mentioned on Github,
> please check the screencast here :
> https://mega.co.nz/#!YwZDVbzB!-87F6cyS1D4nbEjJ2wjVnFnHPQQ4svG0ULSOkC7lgGM

Oops! I forgot to re-add a preventDefault for the checkbutton click event.

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #43)
> And also, I noticed ScreenReader sometimes is not able to turn off, does
> this happen there @Eitan ?

I didn't notice. It will speak and tell you that it is stopping, so it isn't instant. Also, you need to highlight and douple tap the checkbox :)

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #44)
> And I have one more question, if by default we have panel for accessibility,
> should we still use the key in developer panel to show/hide it ? Because I
> think there is no strong reason for now to use that key. Any idea ?

I renamed the key 'accessibility.screenreader-show-settings', and it will show/hide the screen reader sub-section section that should not be on by default for 2.1.
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

EJ,

I pushed another version with your feedback addressed. Thanks!
Attachment #8477644 - Flags: review?(ejchen)
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

@Eitan, it looks nice and r+ !

Thanks for the hard works !!

Please make sure CI is green before merging codes, thanks +++ :P
Attachment #8477644 - Flags: review?(ejchen) → review+
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

I fixed the issue you saw earlier, and the failing tests on TBPL don't look related.

I just rebased and re-pushed in a hope to get a green try.
Attachment #8477644 - Flags: review- → review?(zcampbell)
I won't have a phone to verify it until next week, but I think this PR is reusing the same string for menu label and panel header (colorFilter). If that's the case, it needs to be fixed (see behavior across Gaia Settings).
Depends on: 1062989
Comment on attachment 8477644 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23224

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: There will be no accessibility color contrast options in version 2.1
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): This is not a trivial patch at all, and is an entire refresh of the accessibility panel.
[String changes made]: colorFilter, colorFilter-invert, colorFilter-grayscale, colorFilter-contrast

No excuses :( This is a feature, and not a bug fix. This came in a day after branching, I'm asking for an exception so we could land this in v2.1.
Attachment #8477644 - Flags: approval-gaia-v2.1?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8477644 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #53)
> I won't have a phone to verify it until next week, but I think this PR is
> reusing the same string for menu label and panel header (colorFilter). If
> that's the case, it needs to be fixed (see behavior across Gaia Settings).

Used as a button, a header and a label, definitely need 3 separate strings here.
I guess I should file a follow up bug, and manage to get it uplifted to 2.1 before string freeze
The header part is being fixed in bug 1049824, didn't notice the label just by looking at the code.
Same problem with "screenReader".

Can you add a comment there before it gets landed?
(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #56)
> The header part is being fixed in bug 1049824, didn't notice the label just
> by looking at the code.
> Same problem with "screenReader".
> 
> Can you add a comment there before it gets landed?

Wrong bug number I guess?
Good point about screenReader
Hi please take a look at the latest spec with style update. Thanks!
Attachment #8477254 - Attachment is obsolete: true
(In reply to Théo Chevalier [:tchevalier] from comment #57)
> Wrong bug number I guess?

Yes, bug 1062989
Blocks: 1091166
Blocks: 1095088
Attached video Verify_Video_Flame.MP4
This issue has been verified successfully on Flame 2.1 &2.2.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/10

Flame v2.1 version:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0

Flame v2.2 version:
Gaia-Rev        41b7be7c67167f367c3c4982ff08651d55455373
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/ff4a63d66806
Build-ID        20141126160201
Version         36.0a1
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.