Bug 168447 – AX: when invert colors is on, double-invert video elements in UserAgentStyleSheet

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[James Craig]

AX: when invert colors is on, double-invert certain media elements in UserAgentStyleSheet

Many low-vision users use the "invert colors" setting on macOS and iOS to quickly flip the foreground/background colors of content. This makes it easier for them to read (light-text-on-dark-background instead of dark-text-on-light-background) but has the unfortunate side effect of inverting photographs and videos.

The <video> case seems relatively straightforward to resolve in UASS:

@media (inverted-colors) {
  video { filter: invert(100%); }  /* Poster and playback views double-inverted. */
  video::-webkit-media-controls { filter: invert(100%); } /* Triple-inverted: don't invert the buttons. */
}

But the <img> case needs a little more thought. Here are some pros and cons of possible heuristic solutions. For brevity, I'm only using the <img> selectors, but we'd need to duplicate selectors for the <picture> element.

/* 1. Invert all images. Con: Too many false positives for non-photo images (GIFs of text, decorative images, etc.) */
@media (inverted-colors) { img { filter: invert(100%); } }

/* 2. Invert all images except GIFs. Con: Too many false negatives, but probably okay as a start, as @alt usually indicates "foreground" image. */
@media (inverted-colors) { 
  img { filter: invert(100%); }
  img:not([alt=""]) { filter: invert(0%); } /* Avoid inverting images without @alt. Con: Author override needs to be very explicit. */
}

/* 3. Invert all images except GIFs. Con: Too many false positives with PNGs of text. */
@media (inverted-colors) { 
  img { filter: invert(100%); }
  img:not([src$=".gif"]) { filter: invert(0%); } /* Avoid inverting GIFs. */
}

/* 4. Invert only JPEGs. Pro: Most safe. Con: Possibly too many false negatives since we never know with PNGs. */
@media (inverted-colors) { 
  img[src$=".jpg"], img[src$=".jpeg"] { filter: invert(100%); }
}

[Radar WebKit Bug Importer]

<rdar://problem/30559874>

[James Craig]

We probably want to double-invert *all* images in the Safari reader context, too.

[James Craig]

It appears that @media blocks are not working as I would expect in the user agent style sheet: html.css. I tried implementing the simplest case from the bug as:

@media (inverted-colors) {
  video { filter: invert(100%); }
}

The CSS parser appears to be working (see WebInspector screen shot) but the video filter is applied all the time, regardless of whether the invert colors setting is enabled.

[James Craig]

Created attachment 303174 [details]
dysfunctional

[James Craig]

Created attachment 303175 [details]
Web Inspector screen shot showing @media block from html.css misapplied

[James Craig]

Created attachment 303232 [details]
dysfunctional patch

[James Craig]

Blocked by bug 169245

Comment on attachment 303232 [details]
dysfunctional patch

I think this will be replaced, rather than composed with, any author-specified value for the filter property, which seems wrong.

[James Craig]

Created attachment 326963 [details]
patch

[chris fleizach]

Comment on attachment 326963 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326963&action=review

> Source/WebCore/css/html.css:1223
> +  img, picture, video { filter: invert(100%); }  /* Images, video poster, and playback views double-inverted. */

how did this do on nytimes and other picture heavy websites?
If I didn't do something with background-url(images) I missed out on a lot of images

also doing a straight img invert caught a lot of "UI" images which were unintentional.

> Source/WebCore/css/html.css:1224
> +  video::-webkit-media-controls { filter: invert(100%); } /* Uninvert: Don't invert the buttons. */

do audio controls fall into this category too

[James Craig]

(In reply to chris fleizach from comment #10)
> Comment on attachment 326963 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326963&action=review
> 
> > Source/WebCore/css/html.css:1223
> > +  img, picture, video { filter: invert(100%); }  /* Images, video poster, and playback views double-inverted. */
> 
> how did this do on nytimes and other picture heavy websites?

Very well. Out of the ~"Top 100" I tried, it it significantly improved about 95%. A couple notable exceptions: Ebay uses background images for everything, and LinkedIn uses a less-predictable hybrid of foreground and background images. Even so, these are custom renderings that I would not want to work around in the user style sheet.

> If I didn't do something with background-url(images) I missed out on a lot
> of images

Double-inverting background images would have too many false positives. Initially I was worried that `img` would have too many, but it seems to be working great.

> also doing a straight img invert caught a lot of "UI" images which were
> unintentional.

Are you saying this was a problem for the iOS native implementation (I'd guess it'd be problematic there), or are you talking about a previously attempt at doing this in the Web? If the latter, do you remember where you were seeing the false positives?

> > Source/WebCore/css/html.css:1224
> > +  video::-webkit-media-controls { filter: invert(100%); } /* Uninvert: Don't invert the buttons. */
> 
> do audio controls fall into this category too

I have a better workaround that covers both coming in a later patch. It worked pretty well on popular video sites, but I tweaked it a bit to better cover more sites that use custom video controls.

[chris fleizach]

Comment on attachment 326963 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326963&action=review

> Source/WebCore/css/html.css:1222
> +@media (inverted-colors) {

are you able to add a test for this?

what is the developer story if they don't want an img to be double inverted?

[James Craig]

Created attachment 327043 [details]
patch

[James Craig]

(In reply to chris fleizach from comment #12)
> Comment on attachment 326963 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326963&action=review
> 
> > Source/WebCore/css/html.css:1222
> > +@media (inverted-colors) {
> 
> are you able to add a test for this?

Since mine does not change the default rendering, I don't know of a way to test the inverted state. I'll check to see if Dean was able to do something when he committed the original media features. 

> what is the developer story if they don't want an img to be double inverted?

A single line of CSS.

[Build Bot]

Comment on attachment 327043 [details]
patch

Attachment 327043 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5253919

New failing tests:
http/tests/workers/service/service-worker-clear.html

[Build Bot]

Created attachment 327048 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[James Craig]

Created attachment 328331 [details]
patch

[chris fleizach]

Comment on attachment 328331 [details]
patch

looks good!

[Antoine Quint]

Comment on attachment 328331 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328331&action=review

> Source/WebCore/css/html.css:1223
> +  img, picture, video { filter: invert(100%); }  /* Images and videos double-inverted. */

This should use four spaces to indent.

> LayoutTests/accessibility/smart-invert.html:13
> +let INVERTED_VALUE = "invert(1)";

These should be const.

> LayoutTests/accessibility/smart-invert.html:15
> +function $(id){

You only use this function to get the "result" element. Maybe you could just get a reference to it and use it everywhere?

[Simon Fraser (smfr)]

Comment on attachment 328331 [details]
patch

This patch has the following problems:
1. non-composited filters are slow. It probably takes a few additional milliseconds to paint each filtered image, which makes page painting slower, and will cause more tile flashing.
2. It overrides filters present in the page.  We know that sites use filters for things like image desaturation, and this simply clobbers those filters. We need a solution that doesn't break page content.

[James Craig]

Comment on attachment 328331 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328331&action=review

>> LayoutTests/accessibility/smart-invert.html:15
>> +function $(id){
> 
> You only use this function to get the "result" element. Maybe you could just get a reference to it and use it everywhere?

Yeah. IIRC, I was using it more but refactored it out elsewhere. Probably simpler to just remove it and use document.getElementById in the 3 instances.

[James Craig]

(In reply to Simon Fraser (smfr) from comment #20)
> Comment on attachment 328331 [details]
> patch
> 
> This patch has the following problems:
> 1. non-composited filters are slow. It probably takes a few additional
> milliseconds to paint each filtered image, which makes page painting slower,
> and will cause more tile flashing.

Simon, if I am understanding your perf concern correctly, Antti's previous patch in Bug 169245 addresses it. Media features are only applied in the User Style Sheet after the original paint if completed. 

> 2. It overrides filters present in the page.  We know that sites use filters
> for things like image desaturation, and this simply clobbers those filters.
> We need a solution that doesn't break page content.

If any filter is applied by an author, the default in the UA CSS is ignored. My included test verifies an author-provided filter value is not clobbered. If there is a specific filter you are concerned about, I can add it to the test.

[Simon Fraser (smfr)]

(In reply to James Craig from comment #22)
> (In reply to Simon Fraser (smfr) from comment #20)
> > Comment on attachment 328331 [details]
> > patch
> > 
> > This patch has the following problems:
> > 1. non-composited filters are slow. It probably takes a few additional
> > milliseconds to paint each filtered image, which makes page painting slower,
> > and will cause more tile flashing.
> 
> Simon, if I am understanding your perf concern correctly, Antti's previous
> patch in Bug 169245 addresses it. Media features are only applied in the
> User Style Sheet after the original paint if completed. 

I'm not talking about CSS selector performance, but painting performance. Software filtering is really slow.

> > 2. It overrides filters present in the page.  We know that sites use filters
> > for things like image desaturation, and this simply clobbers those filters.
> > We need a solution that doesn't break page content.
> 
> If any filter is applied by an author, the default in the UA CSS is ignored.
> My included test verifies an author-provided filter value is not clobbered.
> If there is a specific filter you are concerned about, I can add it to the
> test.

Where is the code that does that?

[James Craig]

(In reply to Simon Fraser (smfr) from comment #23)
> (In reply to James Craig from comment #22)
> > My included test verifies an author-provided filter value is not clobbered.
> > If there is a specific filter you are concerned about, I can add it to the
> > test.
> 
> Where is the code that does that?

In the current patch, it verified an author-provided 'none' value on the .no-invert className. I'm about to upload a new patch that verifies an author-provided blur is preserved on the .preserve-filter className.

[James Craig]

Created attachment 328429 [details]
patch

1. Updated selector img:not(picture>img) to address case where fallback image inside picture element had inversion applied twice.
2. Updated based on review from Antoine.
2. Added a second author-provided filter test (blur) based on review from Simon.

[James Craig]

Created attachment 328430 [details]
patch

Forgot to remove an unused function I factored out in the last patch.

[James Craig]

Created attachment 329008 [details]
patch

Changing patch to invert media (video and A/V controls) only. Since IMG/PICTURE inversion needs more discussion, I'll file it as a separate issue.

[Dean Jackson]

Comment on attachment 329008 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329008&action=review

> Source/WebCore/css/html.css:1223
> +    video { filter: invert(100%); }  /* Only videos double-inverted for now. */

Doesn't the rule on the media shadow tree do enough that you don't need this rule?

> LayoutTests/accessibility/smart-invert.html:39
> +        var resultString = filter === value ? "PASS" : "FAIL";
> +        resultString += ": filter for " + el.tagName;
> +        resultString += el.id ? "#" + el.id : "";
> +        resultString += el.className ? "." + el.className : "";
> +        resultString += " is '" + filter + "'.";
> +        resultString += filter !== value ? "Expected: '" + value + "'." : "";
> +        resultString += msg ? " " + msg : "";
> +        resultString += "<br>";

I'm finding `` strings more readable in these circumstances.

[Simon Fraser (smfr)]

Comment on attachment 329008 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329008&action=review

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:133
> +    :host { filter: invert(100%); } /* WebKit native audio and video, with controls. */

You can't assume that shadow DOM is only used for audio and media controls. We may use it for other purposes that don't involve media.

[Simon Fraser (smfr)]

Comment on attachment 329008 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329008&action=review

Can you make a ref test that actually checks that the filters applied as expected?

>> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:133
>> +    :host { filter: invert(100%); } /* WebKit native audio and video, with controls. */
> 
> You can't assume that shadow DOM is only used for audio and media controls. We may use it for other purposes that don't involve media.

Ah, I see that this is inside media-controls.css. Hopefully that does the right thing.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #30)
> Comment on attachment 329008 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329008&action=review
> 
> Can you make a ref test that actually checks that the filters applied as
> expected?
> 
> >> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:133
> >> +    :host { filter: invert(100%); } /* WebKit native audio and video, with controls. */
> > 
> > You can't assume that shadow DOM is only used for audio and media controls. We may use it for other purposes that don't involve media.
> 
> Ah, I see that this is inside media-controls.css. Hopefully that does the
> right thing.

It does because the style is directly injected in the shadow root, so it's only scoped to a media element.

[James Craig]

Comment on attachment 329008 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329008&action=review

>> Source/WebCore/css/html.css:1223
>> +    video { filter: invert(100%); }  /* Only videos double-inverted for now. */
> 
> Doesn't the rule on the media shadow tree do enough that you don't need this rule?

I'll go back to double-check, and add a specific test case to ensure it works with and without @controls.

[James Craig]

(In reply to Simon Fraser (smfr) from comment #30)

> Can you make a ref test that actually checks that the filters applied as
> expected?

Will do.

[James Craig]

Created attachment 330134 [details]
patch

Two separate tests (smart-invert.html and smart-invert-reference.html) to ensure the pixel rendering and dumpAsText values. If there is a good way to combine ref and text tests in a single source HTML file, please advise.

[James Craig]

(In reply to James Craig from comment #32)
> Comment on attachment 329008 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329008&action=review
> 
> >> Source/WebCore/css/html.css:1223
> >> +    video { filter: invert(100%); }  /* Only videos double-inverted for now. */
> > 
> > Doesn't the rule on the media shadow tree do enough that you don't need this rule?
> 
> I'll go back to double-check, and add a specific test case to ensure it
> works with and without @controls.

Simon, I have verified that the :host rule only applies to media elements that use WebKit's controls: For example, video[controls], but not video:not([controls]). We need to keep the rule in html.css to make this work on most major video sites (YouTube, etc) that render their own controls.

[EWS Watchlist]

Comment on attachment 330134 [details]
patch

Attachment 330134 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5804140

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 330137 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330134 [details]
patch

Attachment 330134 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5804259

New failing tests:
accessibility/smart-invert-reference.html
svg/custom/object-sizing-explicit-width.xhtml

[EWS Watchlist]

Created attachment 330140 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[James Craig]

Created attachment 330149 [details]
patch with updated TestExpectations

[Simon Fraser (smfr)]

Comment on attachment 330149 [details]
patch with updated TestExpectations

View in context: https://bugs.webkit.org/attachment.cgi?id=330149&action=review

> Source/WebCore/Modules/modern-media-controls/controls/media-controls.css:134
> +    picture { filter: invert(0%); } /* Don't invert the control buttons. */

filter: none would be better.

[James Craig]

https://trac.webkit.org/changeset/226385/webkit

[Matt Lewis]

The change and test added in this revision have been a flaky image only failure from the time of addition on Sierra Release WK2. 
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fsmart-invert-reference.html

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r226513%20(6741)/results.html
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/6741

diff:
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r226513%20(6741)/accessibility/smart-invert-reference-diffs.html

[Matt Lewis]

Reverted r226385 for reason:

The test introduced with this was a flaky since being added.

Committed r226541: <https://trac.webkit.org/changeset/226541>

[James Craig]

*** Bug 181444 has been marked as a duplicate of this bug. ***

[James Craig]

Created attachment 330873 [details]
patch

new patch uses grayscale rather than blur filter to avoid the pixel flakiness

[James Craig]

Created attachment 330874 [details]
patch

[EWS Watchlist]

Comment on attachment 330874 [details]
patch

Attachment 330874 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6019434

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html

[EWS Watchlist]

Created attachment 330899 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330874 [details]
patch

Attachment 330874 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6019448

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framesize.html

[EWS Watchlist]

Created attachment 330903 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[James Craig]

Failures appear unrelated, as that test does not pass with or without the patch. Ignoring.

[James Craig]

Created attachment 331118 [details]
patch

[WebKit Commit Bot]

Comment on attachment 331118 [details]
patch

Clearing flags on attachment: 331118

Committed r226825: <https://trac.webkit.org/changeset/226825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.