fix(popover): restores drop shadow for nested popovers

[cdransf]

Description

CSS-1066

Restores drop shadow for nested popovers.

Per design and the coach mark component in our Figma library, nested popovers should have drop shadows enabled.

Screenshots

To-do list

• I have read the contribution guidelines.
• I have updated relevant storybook stories and templates.
• I have tested these changes in Windows High Contrast mode.
• If my change impacts other components, I have tested to make sure they don't break.
• If my change impacts documentation, I have updated the documentation accordingly.
• ✨ This pull request is ready to merge. ✨

[changeset-bot]

🦋 Changeset detected

Latest commit: b6bad98

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@spectrum-css/popover	Minor
@spectrum-css/bundle	Patch
@spectrum-css/preview	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

🚀 Deployed on https://pr-3965--spectrum-css.netlify.app

[github-actions]

File metrics

Summary

Total size: 1.42 MB^*

Package	Size	Minified	Gzipped
popover	16.73 KB	16.13 KB	2.04 KB

popover

Filename	Head	Minified	Gzipped	Compared to base
index.css	16.73 KB	16.13 KB	2.04 KB	🔴 ⬆ 0.39 KB
metadata.json	8.16 KB	-	-	🔴 ⬆ 0.04 KB

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

[marissahuysentruyt]

Just putting some of our conversation here for posterity:

I think I agree with you that React's implementation isn't quite what we're looking for. That feels like a much larger refactor, to pull the action menu out of our current markup, just so that it's not part of the "double shadow" thing. If i'm remembering correctly, the shadow on the popover was initially built with filter: drop-shadow(all the drop shadow values..) because we need to incorporate the lil' triangle when popovers have a tip, like the shadow has to follow that weird popover+triangle shape. Otherwise, React's box-shadow doesn't work on it, and doesn't follow the tip shape.

All that being said....if we completely remove that nested popover filter mod, we do end up with double shadows being built. the second screenshot is the nested story on the popover page. This one feels really visible to me- less visible than the one on the action menu in coach mark.

So what if instead, we opt for using box-shadow most of the time, but then move the filter style just to popover-with-tip.

This is a tough one. I'd still want someone from design to take a peek since the box shadow and the filter drop shadow definitely look different 😅 My untrained eyes say the box-shadow actually looks closer to the Figma shadow, but it also feels weird to use box-shadow on some popovers, and filter on others. 🤦‍♀️

My first thought is to refactor the filter at line 72 to use box-shadow instead. That would give us the comparison for popovers with a tip (using filter) and popovers without the tip (box-shadow)

[marissahuysentruyt]

Another note for posterity:

The box shadow (pink) renders smaller than the filter: drop-shadow() (green). However, the box shadow looks to match the Figma drop shadow better- this is where I'd need to get with a designer to ask which they like better. The values are identical for X, Y, blur, and color, but:

• the box-shadow and filter: drop-shadow() use different blur algorithms. 🤦‍♀️
• Apparently, there's also an extra compositing layer with filter: drop-shadow(), which can affect how the blur spreads. 🤦‍♀️
• Maybe there's some bounding container fudging going on which may make the blur extend further. I wonder if the filter is considering the tip as "part" of bounds, and is pretty much adding 8 extra pixels to the bounding container when it shouldn't really, giving the illusion of a larger, softer drop-shadow.

I had everything in dark mode to try to see the shadows better.

[marissahuysentruyt]

Sorry one more comment. I may have a fix, but I'd definitely need some design eyes I think (which we'll get eventually since this is S2). I flipped everything to dark mode and added some wild colors just to experiment and get a better understanding.

So this is with my idea to use box-shadow at line 72 (instead of filter). I left some comments that we can commit or just use here as notes.

We'd define a new custom prop after that filter prop:

/* filter is for popovers with a tip. we divide the blur by 2 to match the box-shadow rendering better. */
--spectrum-popover-filter: drop-shadow(var(--mod-popover-drop-shadow-x, var(--spectrum-popover-drop-shadow-x)) var(--mod-popover-drop-shadow-y, var(--spectrum-popover-drop-shadow-y)) calc(var(--mod-popover-drop-shadow-blur, var(--spectrum-popover-drop-shadow-blur)) / 2) var(--mod-popover-drop-shadow-color, var(--spectrum-popover-drop-shadow-color)));

/* box-shadow is for popovers without a tip. */
--spectrum-popover-box-shadow: var(--mod-popover-drop-shadow-x, var(--spectrum-popover-drop-shadow-x)) var(--mod-popover-drop-shadow-y, var(--spectrum-popover-drop-shadow-y)) var(--mod-popover-drop-shadow-blur, var(--spectrum-popover-drop-shadow-blur)) var(--mod-popover-drop-shadow-color, var(--spectrum-popover-drop-shadow-color));

at line 72ish, replace filter with box-shadow:

...
background-color: var(--mod-popover-background-color, var(--spectrum-popover-background-color));
box-shadow: var(--mod-popover-box-shadow, var(--spectrum-popover-box-shadow));

/* has tip triangle */
&.spectrum-Popover--withTip {
...

Then keep the filter and box-shadow: none styles you have in the --withTip selector block.

That should give us this:

[github-actions]

📚 Branch preview

PR #3965 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3965/index.html.

[cdransf]

Sorry one more comment. I may have a fix, but I'd definitely need some design eyes I think (which we'll get eventually since this is S2). I flipped everything to dark mode and added some wild colors just to experiment and get a better understanding.

So this is with my idea to use box-shadow at line 72 (instead of filter). I left some comments that we can commit or just use here as notes.

We'd define a new custom prop after that filter prop:

/* filter is for popovers with a tip. we divide the blur by 2 to match the box-shadow rendering better. */
--spectrum-popover-filter: drop-shadow(var(--mod-popover-drop-shadow-x, var(--spectrum-popover-drop-shadow-x)) var(--mod-popover-drop-shadow-y, var(--spectrum-popover-drop-shadow-y)) calc(var(--mod-popover-drop-shadow-blur, var(--spectrum-popover-drop-shadow-blur)) / 2) var(--mod-popover-drop-shadow-color, var(--spectrum-popover-drop-shadow-color)));

/* box-shadow is for popovers without a tip. */
--spectrum-popover-box-shadow: var(--mod-popover-drop-shadow-x, var(--spectrum-popover-drop-shadow-x)) var(--mod-popover-drop-shadow-y, var(--spectrum-popover-drop-shadow-y)) var(--mod-popover-drop-shadow-blur, var(--spectrum-popover-drop-shadow-blur)) var(--mod-popover-drop-shadow-color, var(--spectrum-popover-drop-shadow-color));

at line 72ish, replace filter with box-shadow:

...
background-color: var(--mod-popover-background-color, var(--spectrum-popover-background-color));
box-shadow: var(--mod-popover-box-shadow, var(--spectrum-popover-box-shadow));

/* has tip triangle */
&.spectrum-Popover--withTip {
...

Then keep the filter and box-shadow: none styles you have in the --withTip selector block.

Ok! This looks good in the coachmark to my eye, but I'd also love to get design input on it.

(It also looks much better in the popover test view.)

[github-actions]

⚠️ [stylelint] <spectrum-tools/no-unknown-custom-properties> _{reported by reviewdog 🐶}
Custom property --spectrum-popover-border-width not defined

[marissahuysentruyt]

This is looking good to me! I did leave a suggestion for the changeset, since we ended up changing it a little more dramatically that we had initially.

I'm also curious about the stylelint PR comment that --spectrum-popover-border-width is undefined. It was already there during the popover migration, so I'm trying to figure out where that variable went missing...that's a different PR I guess! Thanks for your hard work on this!