New issue
Advanced search Search tips

Issue 648055 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Regression



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 description

UserAgent: 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
 

Comment 1 by f...@opera.com, Sep 19 2016

Components: Blink>Paint
So, something like this then perhaps: https://jsfiddle.net/zL8gffy5/
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/
Cc: senorblanco@chromium.org
Components: -Blink>Paint Blink>Compositing
Owner: jbroman@chromium.org
Status: Assigned (was: Unconfirmed)
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.
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.
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/
Labels: -Pri-2 M-54 M-53 ReleaseBlock-Stable Pri-1
Marking as blocking M54 stable.
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.
Cc: chrishtr@chromium.org pdr@chromium.org ajuma@chromium.org schenney@chromium.org
+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?
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).
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.
Labels: -Pri-1 -ReleaseBlock-Stable Pri-2
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.
It may be possible to do the gradients with SkPaintImageFilter, which is simpler than SkPictureImageFilter, and safe to serialize.
Ok. Is SkPaintImageFilter already serialized? Or does that need to be
implemented?
Also, is it just gradients? What else can be specified in -webkit-box-reflect
and might fail?
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?
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!
Cc: jbroman@chromium.org
Labels: -OS-Windows OS-All
Owner: chrishtr@chromium.org
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.
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
Project Member

Comment 20 by bugdroid1@chromium.org, 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

Labels: -M-53 Merge-Request-54 ReleaseBlock-Stable

Comment 22 by dimu@chromium.org, Sep 30 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 30 2016

Labels: -merge-approved-54 merge-merged-2840
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

Status: Fixed (was: Assigned)
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?
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.
Thanks I will test it and let you know :)
Labels: TE-Verified-54.0.2840.50 TE-Verified-M54
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, 
648055.png
161 KB View Download
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/
br.png
28.7 KB View Download
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/
chrishtr@, want to open a new bug for this, or reopen this one?
Status: Started (was: Fixed)
Status: Fixed (was: Started)
Filed issue 653631 to track the bugs reported in comments 29 and 30.
Closing this bug.

Sign in to add a comment