Summary: | AX: when invert colors is on, double-invert video elements in UserAgentStyleSheet | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | 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
James Craig
2017-02-16 11:02:14 PST
We probably want to double-invert *all* images in the Safari reader context, too. 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. Created attachment 303174 [details]
dysfunctional
Created attachment 303175 [details]
Web Inspector screen shot showing @media block from html.css misapplied
Created attachment 303232 [details]
dysfunctional patch
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.
Created attachment 326963 [details]
patch
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 (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 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? Created attachment 327043 [details]
patch
(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 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 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
Created attachment 328331 [details]
patch
Comment on attachment 328331 [details]
patch
looks good!
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 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 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. (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. (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? (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. 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.
Created attachment 328430 [details]
patch
Forgot to remove an unused function I factored out in the last patch.
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 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 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 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. (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 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. (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. 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.
(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 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 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 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 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
Created attachment 330149 [details]
patch with updated TestExpectations
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. 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 Reverted r226385 for reason: The test introduced with this was a flaky since being added. Committed r226541: <https://trac.webkit.org/changeset/226541> *** Bug 181444 has been marked as a duplicate of this bug. *** Created attachment 330873 [details]
patch
new patch uses grayscale rather than blur filter to avoid the pixel flakiness
Created attachment 330874 [details]
patch
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 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 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 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
Failures appear unrelated, as that test does not pass with or without the patch. Ignoring. Created attachment 331118 [details]
patch
Comment on attachment 331118 [details] patch Clearing flags on attachment: 331118 Committed r226825: <https://trac.webkit.org/changeset/226825> All reviewed patches have been landed. Closing bug. |