https://cs.chromium.org/chromium/src/ash/shelf/shelf_widget.cc?type=cs&q=setbackgroundblur+file:%5Esrc/ash/shelf/+package:%5Echromium$&g=0&l=199

This bug seems to say that the *blur* should be "set on the color, not the layer" but the what the linked CL for the launcher seems to be doing is to set *opacity* on the color rather than the blur layer. Which I think is a little different?

Besides, I'm not quite sure what "setting blur on a color" would really mean?

In the case of the shelf, it doesn't seem like we're setting the opacity on the blur layer, it seems like it's already part of the background color.

But there's definitely something wrong. Attaching the result of 1) the status quo 2) removing the blur, 3) making the shelf fully transparent.

Deleted: 2019_01_15_033937_1198x958_scrot_copy.png 79.8 KB

	2019_01_15_033937_1198x958_scrot_copy.png 79.8 KB View Download

Deleted: 2019_01_15_045535_1198x958_scrot_copy.png 94.1 KB

	2019_01_15_045535_1198x958_scrot_copy.png 94.1 KB View Download

Deleted: 2019_01_15_045939_1198x958_scrot_copy.png 87.4 KB

	2019_01_15_045939_1198x958_scrot_copy.png 87.4 KB View Download

Yeah it does seem like the main fix in CL 1382866 which is to stop using SetOpacity on the layer (and instead convey opacity through a color with an alpha channel) isn't applicable for the shelf since we never call SetOpacity on the layer that has the blur.

@masonfreed since you've been super helpful in diagnosing what caused the problem for the launcher, any idea what we're doing wrong here? The code is around:

https://cs.chromium.org/chromium/src/ash/shelf/shelf_widget.cc?type=cs&q=setbackgroundblur+file:%5Esrc/ash/shelf/+package:%5Echromium$&g=0&l=199

But I don't think this is the same problem as with the launcher.

Assigning to you to make sure you see this, but please reassign to me afterwards :-) Thanks!

Ah, you're right. Sorry for the confusing title, not sure what I was thinking! Opacity should be set on the color, not the blurred layer.

+Dcastagna who could have some insight on this as well.

Issue 922246 has been merged into this issue.

Sorry, just noticed this. Taking a look now...

May be worth a bisect, I'm not 100% convinced mason's change caused this, but I did notice the regression around the same time as their CL.

Could someone please grab this CL and see if it fixes the issue? I can't currently build/run on a ChromeOS device. :-(

https://chromium-review.googlesource.com/c/chromium/src/+/1415171

If this looks ok, that will confirm my suspicion that the problem is in the calculation (or application) of the clipping rect for the backdrop filter. And if so, that CL could actually be landed, pending a better fix for the clip rect. I could take care of that, but it would be super-helpful if we had some tests for the shelf, launcher, etc. Would it be possible to make a layout-test style test for these things? Something that just confirms they look visually like they're supposed to?

@masonfreed Yes, your fix works perfectly. Attaching the "before" and "after" screenshots.

I don't know if it's possible to detect something like this in unit tests, short of doing some kind of image snapshot and comparing to a "gold" state which we don't currently do and may be fairly fragile...

I would guess this is something that would be more feasible at a lower level (same level that your CL is touching)?

Let me know what the best next step is, as far as I'm concerned, landing that pending CL seems to fix this issue completely.

Thanks for looking at this!

Deleted: nopatch.png 399 KB

	nopatch.png 399 KB View Download

Deleted: withpatch.png 393 KB

	withpatch.png 393 KB View Download

Great! I'll fix up that CL and get it landed today.

Can you think about ways to possibly test this stuff? For the web platform, there are WPT tests, and blink's layout tests. Those are quite similar to what we need here - just run the code and make sure the appearance doesn't change. I'm not saying you can reuse that infrastructure, you likely can't. But the idea is quite similar. They are definitely somewhat fragile, depending on how you build them. But they're easy to re-baseline if the appearance changes for a known/planned reason, and you then just have to be careful to look carefully at the output when you re-baseline. Otherwise, it'll watch for you to make sure nothing like this happens in the future. (Particularly with my backdrop-filter changes.) Just a thought!

I can look into testing opportunities, but that's likely going to be an effort larger than what's tracked by this bug :-) My understanding is that the ball is in your court to submit the fix, so I'll reassign to you -- hope that's okay!

No problem, I can take this bug. I'm just waiting on review to land that CL. I chose vollick@ as a reviewer, but would you recommend anyone in particular for this code?

https://chromium-review.googlesource.com/c/chromium/src/+/1415171

Would you mind at least creating a bug to track the need for some sort of testing of the appearance of the shelf/launcher/etc?

No problem, I just filed issue 923416

For reviewers, I'm not sure... Have you tried git cl owners?

 Issue 922357  has been merged into this issue.

Thanks for creating that bug to add tests, I appreciate it.

vollick@ was the first to come up on git cl owners. Let's give it the weekend and hopefully he'll have a chance Monday.

@masonfreed any update? This is a pretty visible bug... Are there other owners who could review your CL?