Issue metadata
Sign in to add a comment
|
CSS box-reflect with a gradient mask doesn't works anymore.
Reported by
gil.gold...@gmail.com,
Sep 18 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36 Steps to reproduce the problem: 1. Create any element with some content or just a background color to be visible. 2. Add do it a CSS property of -webkit-box-reflect. 3. Add a full value with a direction, gradient mask and an offset as: left -2px -webkit-linear-gradient(left, rgb(0, 0, 0) 0%, rgb(0, 0, 0) 100%); What is the expected behavior? The element should have a reflection width a gradient mask. What went wrong? There's no reflection at all, the reflection only works without the gradient mask. Did this work before? Yes All versions til 53.0.2785.116 m Chrome version: 53.0.2785.116 Channel: stable OS Version: 10.0 Flash Version: Shockwave Flash 23.0 r0
,
Sep 19 2016
I see so it does works for regular elements that's good thanks, but not for videos, check this example code and remove the last box-reflect rule leaving only the one with the mask: https://jsfiddle.net/zL8gffy5/1/
,
Sep 19 2016
This is undoubtedly fallout from the conversion to a filter implementation of box-reflect. Assiging under that assumption. The fact that it only fails on video suggests this is more compositor than paint, so changing the component. And since this is probably really a problem with filters, adding some CC people.
,
Sep 19 2016
It seems that <video> is not necessary to trigger this; any compositing hint will do. i.e., "will-change: transform" suffices. I was pretty sure I'd checked this case, so I'm not sure what's going on now.
,
Sep 19 2016
Yes thanks I now see that as you said it isn't just in videos, but also if there is any transform set to a regular element. Here's an example, thanks for notice :) https://jsfiddle.net/zL8gffy5/3/
,
Sep 19 2016
Marking as blocking M54 stable.
,
Sep 19 2016
M54 Stable release is scheduled for the first week of OCT, please have the fix baked/verified in canary and request a merge to M54 ASAP.
,
Sep 20 2016
+a bunch of paint people Okay, here's the deal. With ajuma's help I've figured out what's going on here. The mask is drawn using an SkPictureImageFilter (since the mask can in general be fairly complicated -- it's drawn by NinePieceImagePainter -- and ideally we wouldn't pre-raster it, especially for cases like a gradient). It supports gradients, tiling, nine-piecing, etc. However, we do not permit serialization of SkPictureImageFilter to the browser process, which (due to ubercompositor) is where the filter is ultimately applied. Thus the reflection mask is dropped at the process boundary, and we act as though it were empty (i.e. no reflection is drawn if there was a mask). For non-composited elements, this is applied in the renderer process at tile raster time, and there's no issue. ajuma tells me (and it makes sense to me) that layout tests wouldn't even have caught this, because layout tests do not use a browser compositor. It's not obvious what the right way to deal with this is, but the only fix that would possibly be stable-mergable would be to turn off filter-based box reflect. Unclear whether we'd prefer to do that or not. For a longer term fix, I'm not sure what's best. One approach would be to raster the picture to a bitmap so that can be used instead of the SkPicture. Ultimately I think mus intends to allow sending SkPictures to the place where the final draw happens, but that's a way off yet. WDYT?
,
Sep 20 2016
Yep, that's a problem. Full SkPictures are definitely not hardened enough to deserialize yet. I assume that in the legacy implementation, we were rasterizing these in the renderer process, and not deferring them to the compositor? Would it be possible to do something like that, and provide the input as an SkImageSource input to the filter DAG? Alternatively, could we use something simpler than a full SkPictureImageFilter, whose serialization could be safely validated? Perhaps some kind of SkNinePieceImageFilter/SkNinePieceShader? (Although it does sound like there would be a fair number of parameters required, for gradients, tiling, etc).
,
Sep 20 2016
The legacy approach created a replica mask layer, which was rasterized by cc. In general, it can include image decode (which is one of the big things we didn't want in the browser process yet), so we probably can't solve everything without doing some work in the renderer process. If we want to rasterize it to deal with this, we could let Blink do this (but that will cause graphical imperfections with scaling animations), or have cc do it at some point in its pipeline, or only make some things (like gradients) that we can do safely work and let the others fail. I admit, I'm not a big fan of _any_ of these.
,
Sep 20 2016
Turning off filter-based box reflect is not an option. We need to not go back. I think we should just treat these corner cases as non-blocking bugs.
,
Sep 20 2016
It may be possible to do the gradients with SkPaintImageFilter, which is simpler than SkPictureImageFilter, and safe to serialize.
,
Sep 20 2016
Ok. Is SkPaintImageFilter already serialized? Or does that need to be implemented?
,
Sep 20 2016
Also, is it just gradients? What else can be specified in -webkit-box-reflect and might fail?
,
Sep 20 2016
Yes, I think SkPaintImageFilter does do serialization (has both flatten() and CreateProc() implementations), but this would likely be its first use for gradients in Chrome. SkPaintImageFilter can only do what a single draw with an SkPaint can do. So a gradient shader is doable, but it's doubtful that the 9-patch or tiling stuff could be easily done. (although possibly with SkTileImageFilter, but I'm getting really handwavey). There may be some work required to utilize SkPaintImageFilter in Blink, if we want to "hide" it in the way SkPictureImageFilter is hidden inside GraphicsContext. (Although personally I'm fine with calling it directly from NinePieceImagePainter or whatever if that proves expedient.) I would prefer not to break this use case for M54. Is it really possible not to go back, or just undesirable? Could we fix the gradient case in M55, and switch off filter-based box reflect on the branch in M54?
,
Sep 20 2016
Chris, you can also specify an image mask, which also fails, here's an example: https://jsfiddle.net/zL8gffy5/5/ Thank you guys for working on this, really appreciated!
,
Sep 22 2016
The long-term fix for this is unfortunately likely to be non-trivial. Since paint isn't my primary project anymore and I'm going to be unavailable for much of next week, assigning to chrishtr for retriage.
,
Sep 28 2016
SkPaintImageFilter works great. I implemented it by painting to a bitmap in Blink. This way it works for all content. I don't think the performance is critical for this code anyway, since box reflection is not common. https://codereview.chromium.org/2379453002
,
Sep 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9132e7071ba56be31591b2a9b5b31e6fe0adc3b1 commit 9132e7071ba56be31591b2a9b5b31e6fe0adc3b1 Author: chrishtr <chrishtr@chromium.org> Date: Thu Sep 29 18:29:54 2016 Use SkPaintImageFilter of a bitmap to implement box reflection masks. BUG= 648055 Review-Url: https://codereview.chromium.org/2379453002 Cr-Commit-Position: refs/heads/master@{#421875} [modify] https://crrev.com/9132e7071ba56be31591b2a9b5b31e6fe0adc3b1/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/9132e7071ba56be31591b2a9b5b31e6fe0adc3b1/third_party/WebKit/Source/platform/graphics/BoxReflection.h [modify] https://crrev.com/9132e7071ba56be31591b2a9b5b31e6fe0adc3b1/third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
,
Sep 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf commit 0b2c3c194b805dd3688ec0b2522a0cef5d4573bf Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org> Date: Thu Sep 29 20:04:28 2016 Auto-rebaseline for r421875 https://chromium.googlesource.com/chromium/src/+/9132e7071 BUG= 648055 TBR=chrishtr@chromium.org Review URL: https://codereview.chromium.org/2375153005 . Cr-Commit-Position: refs/heads/master@{#421915} [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/linux/compositing/reflections/masked-reflection-on-composited-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/linux/fast/css/transformed-mask-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/linux/fast/images/color-profile-reflection-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/compositing/reflections/masked-reflection-on-composited-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/css3/blending/effect-background-blend-mode-stacking-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/fast/css/transformed-mask-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/fast/images/color-profile-reflection-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/fast/reflections/reflection-masks-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/fast/images/color-profile-reflection-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/compositing/reflections/masked-reflection-on-composited-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/css3/blending/effect-background-blend-mode-stacking-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/fast/css/transformed-mask-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/fast/images/color-profile-reflection-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/fast/reflections/reflection-masks-expected.png [modify] https://crrev.com/0b2c3c194b805dd3688ec0b2522a0cef5d4573bf/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/fast/images/color-profile-reflection-expected.png
,
Sep 29 2016
,
Sep 30 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/42be9ab0db63120d683fa7d3f34263fc7b3dc37d commit 42be9ab0db63120d683fa7d3f34263fc7b3dc37d Author: Chris Harrelson <chrishtr@chromium.org> Date: Fri Sep 30 21:15:25 2016 Use SkPaintImageFilter of a bitmap to implement box reflection masks. BUG= 648055 Review-Url: https://codereview.chromium.org/2379453002 Cr-Commit-Position: refs/heads/master@{#421875} (cherry picked from commit 9132e7071ba56be31591b2a9b5b31e6fe0adc3b1) Review URL: https://codereview.chromium.org/2387803002 . Cr-Commit-Position: refs/branch-heads/2840@{#608} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/Source/platform/graphics/BoxReflection.h [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp
,
Sep 30 2016
,
Sep 30 2016
Thank you very much, I really appreciate your work, love this awesome feature :) As I don't understand how those things works can you please let me know if I can test it somehow? maybe on Canary? and how can I know when it will go on stable please?
,
Sep 30 2016
It should already be testable on Chrome Canary. I just merged it to the Chrome 54 beta branch, so it will appear in the stable channel in a few weeks. Thanks again for the clear and actionable bug report.
,
Sep 30 2016
Thanks I will test it and let you know :)
,
Oct 5 2016
Tested the issue on windows 7 , Linux Ubuntu 14.04 and Mac 10.11.6 using chrome version 54.0.2840.50 with the URL "https://jsfiddle.net/zL8gffy5/3/" attached in comment #5.Observed the reflection is there for all boxes. Please find the attached screen shot for the same. Adding TE-Verified labels. Thanks,
,
Oct 5 2016
I tested it and it shows up, but when you scroll down the reflections to elements with transforms are slowly getting erased, here's an example I only added height to the body so there will be some scrolling: https://jsfiddle.net/zL8gffy5/6/
,
Oct 5 2016
Another update, if an element has 3D transform then the element itself is also getting erased while scrolling down, he're an example I added another element to the right with "rotate3d" CSS transform: https://jsfiddle.net/zL8gffy5/7/
,
Oct 5 2016
chrishtr@, want to open a new bug for this, or reopen this one?
,
Oct 5 2016
,
Oct 6 2016
Filed issue 653631 to track the bugs reported in comments 29 and 30. Closing this bug.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/42be9ab0db63120d683fa7d3f34263fc7b3dc37d commit 42be9ab0db63120d683fa7d3f34263fc7b3dc37d Author: Chris Harrelson <chrishtr@chromium.org> Date: Fri Sep 30 21:15:25 2016 Use SkPaintImageFilter of a bitmap to implement box reflection masks. BUG= 648055 Review-Url: https://codereview.chromium.org/2379453002 Cr-Commit-Position: refs/heads/master@{#421875} (cherry picked from commit 9132e7071ba56be31591b2a9b5b31e6fe0adc3b1) Review URL: https://codereview.chromium.org/2387803002 . Cr-Commit-Position: refs/branch-heads/2840@{#608} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/Source/platform/graphics/BoxReflection.h [modify] https://crrev.com/42be9ab0db63120d683fa7d3f34263fc7b3dc37d/third_party/WebKit/Source/platform/graphics/filters/SkiaImageFilterBuilder.cpp |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by f...@opera.com
, Sep 19 2016