Contrast for accessibility

VERIFIED FIXED in Firefox OS v2.1

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

26 Branch
mozilla34
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(feature-b2g:2.1, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

Bug 1016539 landed inverting colors and grayscale effect.  This bug will cover some follow up changes and adding contrast.

Contrast = 0 - unchanged color
Contrast = -1 - everything is mid gray (0.5)
Contrast > 0 - increasing contrast - values < 0.5 decrease, values > 0.5 increase, linearly, until we hit everything as black or white (because of clipping.)
Assignee: nobody → milan
Blocks: 1016539
Created attachment 8475618 [details] [diff] [review]
Add contrast computation and rename some of the prefs. r=mwoodrow
Attachment #8475618 - Flags: review?(matt.woodrow)
Attachment #8475618 - Attachment is patch: true
Created attachment 8475619 [details] [diff] [review]
Add contrast computation and rename some of the prefs. r=mwoodrow

Add more comments as to what we're actually doing...
Attachment #8475618 - Attachment is obsolete: true
Attachment #8475618 - Flags: review?(matt.woodrow)
Attachment #8475619 - Flags: review?(matt.woodrow)
Comment on attachment 8475619 [details] [diff] [review]
Add contrast computation and rename some of the prefs. r=mwoodrow

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +481,5 @@
> +    matrix._33 = contrastPlus1*matrix._33;
> +    matrix._51 = contrastPlus1*matrix._51 - halfContrast;
> +    matrix._52 = contrastPlus1*matrix._52 - halfContrast;
> +    matrix._53 = contrastPlus1*matrix._53 - halfContrast;
> +  }

Took me a while to convince myself this was correct. Markus is adding an operator* for Matrix5x4 in bug 1055661, I'd prefer if you used that.

@@ +523,5 @@
>      this->Dump(layersPacket);
>      LayerScope::SendLayerDump(Move(packet));
>    }
>  
> +  if (gfxPrefs::LayersEffectInvert() || gfxPrefs::LayersEffectGrayscale() || gfxPrefs::LayersEffectContrast() != 0.0) {

Add some newlines in here to keep us under 80-ish characters and stop it wrapping on my screen :) Same below.
Attachment #8475619 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8475619 [details] [diff] [review]
> Add contrast computation and rename some of the prefs. r=mwoodrow
> 
> Review of attachment 8475619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +481,5 @@
> > +    matrix._33 = contrastPlus1*matrix._33;
> > +    matrix._51 = contrastPlus1*matrix._51 - halfContrast;
> > +    matrix._52 = contrastPlus1*matrix._52 - halfContrast;
> > +    matrix._53 = contrastPlus1*matrix._53 - halfContrast;
> > +  }
> 
> Took me a while to convince myself this was correct. Markus is adding an
> operator* for Matrix5x4 in bug 1055661, I'd prefer if you used that.

Nice!  I had it all written out on paper to convince myself of the previous ones as well.  I'll make this depend on bug 1055661 and use that.
> 
> @@ +523,5 @@
> >      this->Dump(layersPacket);
> >      LayerScope::SendLayerDump(Move(packet));
> >    }
> >  
> > +  if (gfxPrefs::LayersEffectInvert() || gfxPrefs::LayersEffectGrayscale() || gfxPrefs::LayersEffectContrast() != 0.0) {
> 
> Add some newlines in here to keep us under 80-ish characters and stop it
> wrapping on my screen :) Same below.

Sounds good.

Thanks for the quick turnaround.
Depends on: 1055661
feature-b2g: --- → 2.1
Created attachment 8476216 [details] [diff] [review]
Add contrast computation and rename some of the prefs, rename some methods and add comments for clarity.. r=mwoodrow

I'll ignore the r+ on the previous patch and ask for another review, as I also made changes beyond the suggested ones (albeit fairly cosmetic.)  I just don't want to go too far with asserts, explicit method names, etc., so a sanity check would help.

https://tbpl.mozilla.org/?tree=Try&rev=4b140534a5a1
Attachment #8475619 - Attachment is obsolete: true
Attachment #8476216 - Flags: review?(matt.woodrow)
Note: the patch above does require bug 1055661 to land.
Comment on attachment 8476216 [details] [diff] [review]
Add contrast computation and rename some of the prefs, rename some methods and add comments for clarity.. r=mwoodrow

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +469,5 @@
> +    // B' = 1 - B
> +    Matrix5x4 colorInvertMatrix(-1, 0, 0, 0,
> +                                0, -1, 0, 0,
> +                                0, 0, -1, 0,
> +                                0, 0, 0, 1,

Alignment!

@@ +471,5 @@
> +                                0, -1, 0, 0,
> +                                0, 0, -1, 0,
> +                                0, 0, 0, 1,
> +                                1, 1, 1, 0);
> +    effectMatrix = effectMatrix * colorInvertMatrix;

You could add operator*= to Matrix5x4 here, doesn't matter much though.

@@ +539,5 @@
>  
> +  /** Our more efficient but less powerful alter ego, if one is available. */
> +  nsRefPtr<Composer2D> composer2D;
> +
> +  // We don't need composer2D if we have layer effects

'can't use' rather than 'don't need'
Attachment #8476216 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8476216 [details] [diff] [review]
> ...
> > +    effectMatrix = effectMatrix * colorInvertMatrix;
> 
> You could add operator*= to Matrix5x4 here, doesn't matter much though.

Already suggested it in bug 1055661, lets see if Markus bites :)

Updated

4 years ago
Flags: in-moztrap?
Created attachment 8478326 [details] [diff] [review]
Add contrast computation and rename some of the prefs, rename some methods and add comments for clarity.. r=mwoodrow

Address review comments, carry r=mattwoodrow
Attachment #8476216 - Attachment is obsolete: true
Attachment #8478326 - Flags: review+
Link to moztrap test case:
https://moztrap.mozilla.org/manage/case/14505/
Flags: in-moztrap? → in-moztrap+
https://hg.mozilla.org/mozilla-central/rev/ef8d62bc4aa6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
This is also affected by Bug 1057461
Depends on: 1057461
No longer depends on: 1055661

Updated

4 years ago
No longer depends on: 1057461

Updated

4 years ago
Depends on: 1057461
This issue has been verified on Flame 2.2 and Flame 2.1:

Flame 2.2 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141006040204
Gaia: 470826d13ae130a5c3d572d1029e595105485fb0
Gecko: e0d714f43edc
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141006000205
Gaia: 778ebac47554e1c4b7e9a952d73e850f58123914
Gecko: c4a4b04c617c
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Contrast on the Accessibility menu is implemented and fully functional.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1: --- → verified
status-b2g-v2.2: --- → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Updated

4 years ago
Blocks: 1095088

Updated

4 years ago
Blocks: 1091166
You need to log in before you can comment on or make changes to this bug.