feat(Popover): dev-mode warning when display utility overrides native popover hide#464
Conversation
After mount, clone the content element without its [popover] attribute
so neither the UA closed-state rule nor the existing
`[popover]:not(:popover-open){display:none!important}` override applies.
If the clone's computed display is not 'none' or 'block', a utility class
(flex, grid, …) is set directly on Popover.Content and a scoped logger
warning is emitted pointing users to a wrapper-element solution.
Guard is tree-shaken in production via `typeof __DEV__ !== 'undefined' &&
__DEV__`, is SSR-safe via the IN_BROWSER guard, and is covered by two new
unit tests that mock getComputedStyle.
Closes vuetifyjs#448
|
✅ Changeset found — this change will be included in the next release. Thanks! |
|
commit: |
|
I wonder if this would be better served as a warning to the user. It's definitely a good conversation to have regarding when/where/why we would add behavior like this to a component. @J-Sek @AndreyYolkin @Haviles04 |
| probe.remove() | ||
|
|
||
| if (display && display !== 'none' && display !== 'block') { | ||
| const msg = `[Popover.Content] A display utility class (detected: "${display}") is set directly on the content element. This conflicts with the native [popover] closed-state hide. Move layout classes (flex, grid, etc.) to a wrapper element inside the slot instead.` |
There was a problem hiding this comment.
code probes computed style, not class, so "A display utility class ..." is not precise and might be misleading.
|
It is hard to take a stance - scenario suggests, the menu is being rendered, so why not just assume it is likely to be visible, making console warning redundant. It only potentially serves people that ship straight to Prod without QA and skipping manual verification locally. That's also a bit weak.. they probably skip opening browser and running affected web page altogether, so console logs won't change much. I would prefer ESLint rule, but can't help by notice testing against computed style covers more ground. The only issue is potential performance overhead. Vue DevTools already hit hard, so maybe it does not matter, but we could make sure by having 20x500 table rows here each table has some kind of dropdown for context menu or value selector. We can also make it lighter by setting empty inner body My vote would be to skip this case. Simply rely on class making the popover visible. |
| // Clone the element without [popover] so neither the UA closed-state rule | ||
| // nor our [popover]:not(:popover-open){display:none!important} override |
There was a problem hiding this comment.
I don't think v0 ships these styles + if UA makes the display hidden, then it wins over class and there is no issue to warn about - i.e. cloning is adding friction instead of solving real problem.
|
I agree with Jacek that an ESLINT rule is preferable, this feels heavy just to say "hey maybe don't do that" |
Summary
Closes #448.
When a layout utility class (
flex,grid,inline-flex, etc.) is placed directly on<Popover.Content>, it can conflict with the UA[popover]:not(:popover-open) { display: none }rule that hides a closed popover. This was the root cause of #419; the CSS-layer fix landed in #419/398, but there was no developer feedback to prevent the pattern from reappearing.What this PR adds
PopoverContent.vue—onMountedguard (tree-shaken to nothing in production viatypeof __DEV__ !== 'undefined' && __DEV__):[popover]attribute, so neither the UA closed-state rule nor the component's own[popover]:not(:popover-open){display:none!important}override applies to the clone.getComputedStyle(clone).display.noneorblock(i.e. the default div display), auseLogger('PopoverContent').warn(...)is emitted, naming the detected value and pointing to the fix: wrap layout classes in a child element.The guard is also SSR-safe via the
IN_BROWSERearly return.Tests
Two new cases in
index.test.tsundercontent > dev warning:getComputedStylereturns'flex'for the off-screen probe (simulated viavi.spyOn).getComputedStylereturns the happy-dom default''(no utility class present).Before / After