Bug 168447

Summary: AX: when invert colors is on, double-invert video elements in UserAgentStyleSheet
Product: WebKit Reporter: James Craig <jcraig>
Component: CSSAssignee: James Craig <jcraig>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cfleizach, commit-queue, dino, ews-watchlist, graouts, jcraig, jlewis3, koivisto, mitz, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 169245    
Bug Blocks: 181281    
Attachments:
Description Flags
dysfunctional
none
Web Inspector screen shot showing @media block from html.css misapplied
none
dysfunctional patch
none
patch
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
patch
simon.fraser: review-
patch
none
patch
none
patch
simon.fraser: review+
patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
patch with updated TestExpectations
simon.fraser: review+
patch
none
patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews113 for mac-sierra
none
patch none

Description James Craig 2017-02-16 11:02:14 PST
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%); }
}
Comment 1 Radar WebKit Bug Importer 2017-02-16 11:03:04 PST
<rdar://problem/30559874>
Comment 2 James Craig 2017-02-16 19:29:00 PST
We probably want to double-invert *all* images in the Safari reader context, too.
Comment 3 James Craig 2017-03-01 22:35:43 PST
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.
Comment 4 James Craig 2017-03-01 22:39:34 PST
Created attachment 303174 [details]
dysfunctional
Comment 5 James Craig 2017-03-01 22:48:28 PST
Created attachment 303175 [details]
Web Inspector screen shot showing @media block from html.css misapplied
Comment 6 James Craig 2017-03-02 13:44:06 PST
Created attachment 303232 [details]
dysfunctional patch
Comment 7 James Craig 2017-03-29 19:09:33 PDT
Blocked by bug 169245
Comment 8 mitz 2017-07-19 19:21:10 PDT
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.
Comment 9 James Craig 2017-11-14 20:20:27 PST
Created attachment 326963 [details]
patch
Comment 10 chris fleizach 2017-11-15 00:31:05 PST
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
Comment 11 James Craig 2017-11-15 17:46:19 PST
(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.
Comment 12 chris fleizach 2017-11-15 17:48:51 PST
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?
Comment 13 James Craig 2017-11-15 18:35:54 PST
Created attachment 327043 [details]
patch
Comment 14 James Craig 2017-11-15 18:45:50 PST
(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.
Comment 15 Build Bot 2017-11-15 20:11:03 PST
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
Comment 16 Build Bot 2017-11-15 20:11:05 PST
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
Comment 17 James Craig 2017-12-03 23:09:44 PST
Created attachment 328331 [details]
patch
Comment 18 chris fleizach 2017-12-03 23:13:02 PST
Comment on attachment 328331 [details]
patch

looks good!
Comment 19 Antoine Quint 2017-12-04 02:17:39 PST
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?
Comment 20 Simon Fraser (smfr) 2017-12-04 08:07:24 PST
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.
Comment 21 James Craig 2017-12-04 18:25:08 PST
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.
Comment 22 James Craig 2017-12-04 18:36:23 PST
(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.
Comment 23 Simon Fraser (smfr) 2017-12-04 19:25:06 PST
(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?
Comment 24 James Craig 2017-12-04 19:32:00 PST
(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.
Comment 25 James Craig 2017-12-04 19:38:17 PST
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.
Comment 26 James Craig 2017-12-04 20:11:49 PST
Created attachment 328430 [details]
patch

Forgot to remove an unused function I factored out in the last patch.
Comment 27 James Craig 2017-12-11 11:46:02 PST
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.
Comment 28 Dean Jackson 2017-12-11 12:04:50 PST
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.
Comment 29 Simon Fraser (smfr) 2017-12-11 13:24:31 PST
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.
Comment 30 Simon Fraser (smfr) 2017-12-11 13:32:44 PST
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.
Comment 31 Antoine Quint 2017-12-11 13:50:57 PST
(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.
Comment 32 James Craig 2017-12-11 17:22:00 PST
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.
Comment 33 James Craig 2017-12-11 17:23:37 PST
(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.
Comment 34 James Craig 2017-12-22 11:25:10 PST
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.
Comment 35 James Craig 2017-12-22 11:28:46 PST
(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.
Comment 36 EWS Watchlist 2017-12-22 12:22:02 PST
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
Comment 37 EWS Watchlist 2017-12-22 12:22:03 PST
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
Comment 38 EWS Watchlist 2017-12-22 12:51:11 PST
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
Comment 39 EWS Watchlist 2017-12-22 12:51:13 PST
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
Comment 40 James Craig 2017-12-22 15:20:18 PST
Created attachment 330149 [details]
patch with updated TestExpectations
Comment 41 Simon Fraser (smfr) 2018-01-03 11:17:59 PST
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.
Comment 42 James Craig 2018-01-03 17:46:12 PST
https://trac.webkit.org/changeset/226385/webkit
Comment 44 Matt Lewis 2018-01-08 14:36:52 PST
Reverted r226385 for reason:

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

Committed r226541: <https://trac.webkit.org/changeset/226541>
Comment 45 James Craig 2018-01-09 11:15:56 PST
*** Bug 181444 has been marked as a duplicate of this bug. ***
Comment 46 James Craig 2018-01-09 19:59:46 PST
Created attachment 330873 [details]
patch

new patch uses grayscale rather than blur filter to avoid the pixel flakiness
Comment 47 James Craig 2018-01-09 20:03:51 PST
Created attachment 330874 [details]
patch
Comment 48 EWS Watchlist 2018-01-10 06:46:31 PST
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
Comment 49 EWS Watchlist 2018-01-10 06:46:33 PST
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
Comment 50 EWS Watchlist 2018-01-10 07:06:54 PST
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
Comment 51 EWS Watchlist 2018-01-10 07:06:56 PST
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
Comment 52 James Craig 2018-01-10 07:18:10 PST
Failures appear unrelated, as that test does not pass with or without the patch. Ignoring.
Comment 53 James Craig 2018-01-11 14:18:02 PST
Created attachment 331118 [details]
patch
Comment 54 WebKit Commit Bot 2018-01-11 17:17:04 PST
Comment on attachment 331118 [details]
patch

Clearing flags on attachment: 331118

Committed r226825: <https://trac.webkit.org/changeset/226825>
Comment 55 WebKit Commit Bot 2018-01-11 17:17:06 PST
All reviewed patches have been landed.  Closing bug.