Closed Bug 1227216 Opened 4 years ago Closed 4 years ago

Strange inset shadow in FF 45.0a1 (2015-11-22)

Categories

(Core :: Graphics, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: sergen-vas, Assigned: mchang)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(5 files)

Attached image ff-shadow-issue.png
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151122004109

Steps to reproduce:

1. Create first element with `box-shadow: inset 0 0 1px -3px rgba(0,0,10,0.25)`.
2. Second one will got `box-shadow: inset 0 5px 1px -2px rgba(0,0,10,0.25)`

Example: http://jsfiddle.net/SerGen/uv6r12hy/


Actual results:

1. Shadow covers all element
2. Offset clears shadow


Expected results:

1, 2. Shadow must appear only on top of element with little blur.
I can't reproduce the issue with the 1st square but there is a difference with the 2nd element. (see my screenshot)

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f2cc5afecb1f3553d4f400b88bdfe2d3991ef8c0&tochange=3c837b3a38fa15b655c4115cb95eda741b55a819
Flags: needinfo?(mchang)
Mason is on PTO, any thoughts, Markus?
Component: Untriaged → Graphics
Flags: needinfo?(mstange)
Keywords: regression
Product: Firefox → Core
Attached image ff45_24112015.png
ff45_24112015.png

Still can reproduce issue with first in ff45.0a1 (2015-11-24).
Whiteboard: [MemShrink], gfx-noted
Assignee: nobody → mchang
Flags: needinfo?(mstange)
Flags: needinfo?(mchang)
What's interesting about the first box shadow is that both Chrome / Safari /Edge show an empty box instead of any box shadow, so I think our implementation in Firefox 42 is also wrong.
The first box shadow in the jsfiddle had a bug where we'd paint the min inset box shadow as a draw surface at [1]. This is because the spread radius was negative, and after extending the destination, the slice would become 0, causing us to stretch the min inset blur to the final destination rect. This patch will only draw the surface if the destination rect is the size of the source rect, which should only happen if the min rects are bigger than the dest rects, since we snap to those sizes at [2].

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp#1050
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxBlur.cpp?from=gfxBlur.cpp#847
Attachment #8694261 - Flags: review?(mstange)
The second box shadow has a bug because the offset attached to the box-shadow is larger than the min inset blur's size. Before the inset box shadow rewrite, we would fill the area between the inner and outer rect with the shadow color, then blur the edges. The shadow offset would move the inner rect by the offset, then fill the difference.

Now, we create a min rect, blur it, and position the blur to the offset location and draw there. This is normally fine. However, in this case, the offset was large enough to push the min inner rect further than the slice area we'd normally paint onto the destination.  e.g. Imagine a 100x100 div with a blur radius of 1 and an offset of 50px. We'd create a very small min box shadow but the offset would be much bigger than the box shadow size. The area between the actual blur and the div needs to be filled. In these cases, this patch falls back to just blurring the destination rect.
Attachment #8694262 - Flags: review?(mstange)
Clearing MemShrink for now, :BenWa can add it back if it was intentional.
Whiteboard: [MemShrink], gfx-noted → gfx-noted
Attachment #8694261 - Flags: review?(mstange) → review+
Comment on attachment 8694262 [details] [diff] [review]
Fallback to blur dest rect if a large shadow offset

Review of attachment 8694262 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose it would be nice if our minimum surface didn't have to care about what the outer shape looks like it and where it's positioned in it, but that's for another bug. For example, we could use the minimum surface to only draw the blurred parts of the shadow, and then use solid color FillRects to fill out the rest of the outer shape.
Attachment #8694262 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/b9844966d2fb
https://hg.mozilla.org/mozilla-central/rev/46c095a4311b
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1231553
Duplicate of this bug: 1246821
Duplicate of this bug: 1250890
Version: 45 Branch → 44 Branch
Depends on: 1250947
Depends on: 1256403
Depends on: 1257054
No longer depends on: 1257054
Depends on: 1258132
You need to log in before you can comment on or make changes to this bug.