feat(dev-validation): apply shared validation helpers across all Gen2 components - #6510
Conversation
…elpers First representative batch applying the shared dev-validation helpers from the foundation branch: - Badge: variant enum check -> validateEnum; outline/non-semantic- variant combo -> warnIf. - Tabs: keyboard-activation/density/direction enum checks -> validateEnum (kept in their property setters alongside the existing fallback-value logic); required accessible-label check -> warnIf. - IllustratedMessage: size/orientation enum checks -> validateEnum; heading slot allowed-children check -> validateAllowedChildren (previously a one-off inline tag allowlist). Also fixes the docs URLs in these three components (and this session's style-guide examples) from the stale opensource.adobe.com/spectrum-web- components/components/* pattern to the actual Gen2 docs site: spectrum-web-components.adobe.com/?path=/docs/components-*--docs. No new required-slot check was added for IllustratedMessage's heading slot: an existing test explicitly asserts no warning fires when it's empty, so that's left alone rather than overridden. All existing tests for these three components pass unchanged (97 tests across badge/tabs/illustrated-message/dev-validation), since the new helpers preserve enough of each original message's wording for the existing assertions to still match.
|
VARIANTS_SEMANTIC has a fixed initializer directly on BadgeBase (SHARED API section), unlike VARIANTS (API TO OVERRIDE, no initializer, genuinely meant to differ per concrete subclass per its JSDoc). Going through this.constructor for VARIANTS_SEMANTIC was redundant indirection; the already-imported BADGE_VARIANTS_SEMANTIC constant is identical. The VARIANTS lookup stays, since that one is by design abstract and subclass-supplied.
📚 Branch Preview Links🔍 Gen1 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
… static" This reverts commit 2dc721c.
…eter - ProgressCircle: light-DOM-children deprecation and missing- accessible-name checks -> warnIf. New: staticColor enum check via validateEnum (previously undeclared/unvalidated despite STATIC_COLORS existing for styling purposes). - LinearProgressMixin (shared by Meter): value-out-of-range and missing-accessible-name checks -> warnIf, keeping the mixin's own instance-level _hasWarned* flags as-is (they implement a distinct "warn once per bad-state entry, reset on recovery" policy layered on top of the shared warn() dedup, not something the helpers replace). New: labelPosition and staticColor enum checks via validateEnum (previously undeclared/unvalidated). - Meter: variant enum check -> validateEnum. All existing tests for progress-circle and meter pass unchanged (48 tests).
- Button: pending+disabled combo and icon-only-without-label checks -> warnIf (the latter now tagged type: 'accessibility', matching how every other accessible-name check in this effort is categorized). - ActionButton: new staticColor enum check via validateEnum (previously undeclared/unvalidated; no core base class owns this property, so the check lives directly on the swc concrete class). - Card: variant/density enum checks -> validateEnum; title-as-link- without-a-link-element check -> warnIf. - Tooltip: dangling for="" reference check -> warnIf. New: variant and placement enum checks via validateEnum (both had a VARIANTS/PLACEMENTS static already declared but never actually validated against). color-loupe was reviewed and needs no changes (boolean + free-text CSS color property only, no enums/required/slots). All existing tests for action-button, button, button-group, tooltip, and card pass unchanged (142 tests).
- Asset: variant enum check -> validateEnum. - AlertBanner: variant enum check (in the property setter) -> validateEnum. - StatusLight: 'accent' deprecation and disabled-attribute-removed checks -> warnIf; variant enum check -> validateEnum. Fixed the variant check's stale opensource.adobe.com URL. - Popover: dangling for="" reference check and required accessible- label-on-show check -> warnIf; placement enum check -> validateEnum. New: size enum check via validateEnum (VALID_SIZES existed but was never validated). Updated one test (status-light) that asserted the exact old warning message text via .toBe() rather than .toContain(); switched it to match the new standardized validateEnum message format instead of hand-tuning the helper's wording to preserve a stale test string. All existing tests for asset, alert-banner, status-light, and popover pass (90 tests).
- Divider: staticColor enum check -> validateEnum. - Avatar: missing-alt accessibility check -> warnIf. New: numeric size enum check via warnIf (VALID_SIZES existed and the setter already fell back to the default on an invalid value, but never warned about it). - ButtonGroup: orientation and align enum checks -> validateEnum. Icon was reviewed and needs no changes (free-text label, no required slot semantics beyond a graceful no-op when nothing is slotted). All existing tests for divider, avatar, icon, and button-group pass unchanged (78 tests).
ACCORDION_DENSITIES existed as the canonical valid-values source but was never actually validated anywhere. Added via validateEnum. ColorLoupe and ColorHandle were both already reviewed earlier this effort and confirmed to need no changes (booleans + free-text CSS color only). All existing tests for accordion, color-handle, and color-loupe pass unchanged (67 tests).
rubencarvalho
changed the title
…n/feat-dev-warning-validation-components
…n/feat-dev-warning-validation-components
…n/feat-dev-warning-validation-components
…n/feat-dev-warning-validation-components
…n/feat-dev-warning-validation-components
…//github.com/adobe/spectrum-web-components into ruben/feat-dev-warning-validation-components
| @@ -68,16 +69,12 @@ export abstract class MeterBase extends LinearProgressMixin( | |||
| protected override willUpdate(changes: PropertyValues): void { | |||
There was a problem hiding this comment.
In my third PR, I want to refactor this to an update or updated
I think we need more conversations in regards to our fallback behaviors.
| warnIf( | ||
| this, | ||
| true, | ||
| `<${this.localName}> requires an accessible name. A default label of "${ProgressCircleBase.DEFAULT_LABEL}" has been applied, but a more specific label should be provided via:`, |
There was a problem hiding this comment.
Should we be coercing warnings and mutating the consumers input? This is another candidate for a refactor PR that removes these coercions and puts the responsibility of correctly setting up the components on the consumer.
| } | ||
| warnIf( | ||
| this, | ||
| !this.accessibleLabel, |
There was a problem hiding this comment.
warnIf(this, !this.accessibleLabel, …) only reads accessibleLabel. If a consumer names the tablist via aria-labelledby (or aria-label) on the host instead, this still warns even though the element has an accessible name, a false positive. Do we support those as valid naming paths for Tabs? If so, the condition should treat them as satisfied, example:
!this.accessibleLabel &&
!this.hasAttribute('aria-label') &&
!this.hasAttribute('aria-labelledby')
|
|
||
| @state() | ||
| private _ariaExpanded?: string; | ||
| private _ariaExpanded?: 'true' | 'false'; |
miwha-adobe
added
Status:Ready for review
| } | ||
| this._hasWarnedNoAccessibleName = true; | ||
| window.__swc?.warn( | ||
| warnIf( |
There was a problem hiding this comment.
Dropped _hasWarnedNoAccessibleName here. warnIf's dedup is permanent for the session (keyed by localName:type:level), so fix-then-re-break within a session no longer re-warns. LinearProgressMixin kept its _hasWarned* and the same fix should apply here. ✨
There was a problem hiding this comment.
Alrighttt, so I dug into this a bit more, and both _hasWarned checks (LinearProgressMixin and Dropzone) don't actually seem to do anything 🤔.
The logic reads like it should recall the warning, but the dedup underneath swallows any identical message that already fired this session. That's 2nd gen wide, not specific to these two, so the flag never gets to re-surface anything.
For Dropzone there's a second reason on top of that: the accessible name lives in aria-label / aria labelledby, which aren't reactive, so changing them after connect doesn't trigger a re-render and the check never re-runs. The changes.has('dragged') bit looks like a re-check but it's really just a side effect of dragging, not a direct check that the name changed.
(For LinearProgressMixin the inputs are reactive, so it does re-render, but the dedup still swallows the repeat. The one warning that reappears there is value-out-of-range, and only because the value is in the message text, so it's technically a different message. That's not the flag doing anything either.)
For PR 3 I'd like to add a controller that watches aria changes so we surface the errors we expect when the name actually changes. Caveat: same message still won't re-surface because of dedup. Open to discussing whether re-surfacing is a pattern we want to support, but these one-offs feel like attempts to work around this that don't really succeed.
There was a problem hiding this comment.
Added Dropzone and LinearProgressMixin refactor here: #6561
| return; | ||
| } | ||
| window.__swc.warn( | ||
| warnIf( |
There was a problem hiding this comment.
hasMeaningfulLightDomChildren(this) is evaluated as an arg, so it runs unconditionally every updated() — in prod too. Lost the old isDebug() early-return. ✨
There was a problem hiding this comment.
this too, great catch, ty!
Coverage Report for CI Build 30847227211Warning No base build found for commit Coverage: 96.239%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…//github.com/adobe/spectrum-web-components into ruben/feat-dev-warning-validation-components
| @@ -140,15 +141,12 @@ export abstract class ButtonBase extends SizedMixin(SpectrumElement, { | |||
|
|
|||
| protected override update(changedProperties: PropertyValues): void { | |||
| super.update(changedProperties); | |||
There was a problem hiding this comment.
I'm still trying to make sure I follow the docs correctly, but super.update(changedProperties) is called first, then the check. Per the docs the check should run before super.update()?
There was a problem hiding this comment.
My understanding is that it depends on when we want to call the warning. This one is on hasIcon, which are slot derived and so they aren't reliable until the slot is rendered, which is why it is after super.update()
| @@ -475,6 +475,24 @@ export abstract class TooltipBase | |||
|
|
|||
| protected override updated(changedProperties: PropertyValues): void { | |||
| super.updated(changedProperties); | |||
There was a problem hiding this comment.
Actually... I'm not sure what to think... Shouldn't we want to let any super class finish first before validation?
There was a problem hiding this comment.
I think that is a discussion worth having. Certainly the ones that rely on a render should (slots, etc). Do we lose anything from moving all warnings to after the render? It would keep a consistent pattern. The benefits to a pre-render seem somewhat marginal, though we would need to update our docs to support this pattern.
6 participants
Description
Applies the shared dev-validation helpers introduced in #6508 (the foundation branch this PR targets) across the Gen2 component set — core base classes, shared mixins, and swc subclasses.
What this does:
Retrofits ad hoc checks onto the shared helpers. Replaces hand-rolled
window.__swc.warn+includes()call sites withvalidateEnum/warnIf/validateRequiredSlot/validateAllowedChildren. This reaches beyond the core base classes into the shared mixins (PendingMixin,LinearProgressMixin) and the concrete swc subclasses (Button,CloseButton,ActionButton), which each carried their own inline checks.Adds validation where there was none — Accordion (
density), Popover (size), Avatar (numericsize), plus new enum checks on Tooltip (variant/placement), ActionButton (staticColor), and ProgressCircle / LinearProgressMixin (staticColor/labelPosition).Guards expensive checks with
isDebug(). Where a condition is a DOM traversal (ProgressCircle's light-DOM check, Card'sactions-slot check), the call site short-circuits when validation is off, so the traversal isn't paid on every update.Fixes doc URLs on every touched file (and the style-guide examples from the foundation branch): stale
opensource.adobe.com/spectrum-web-components/components/*→ the Gen2 docs sitespectrum-web-components.adobe.com/?path=/docs/components-*--docs.Fixes 7 missing helper imports (Accordion, AlertBanner, Asset, Avatar, Badge, ButtonGroup, Card): used but not imported, so they transpiled under Vite but threw
ReferenceErrorat runtime — onlytsc --noEmitcaught them.Full per-component breakdown (all 20)
staticColor/labelPositionenum checks addedstaticColor(ActionButton),variant/placement(Tooltip) enum checks added; Cardactions-slot check retrofittedsizeenum check added (Popover)sizecheck added (Avatar)window.__swc.warncall sites retrofitted ontowarnIf/validateEnumdensityenum check added (previously undeclared)Motivation and context
Extends the helper-based pattern from the foundation branch across the entire component set — including the mixins and subclasses that validated inline — completing the "retrofit existing ad hoc call sites" goal and closing the gap on components that had no dev-mode validation at all.
Related issue(s)
Screenshots (if appropriate)
N/A — no visual changes; only dev-mode console warning behavior.
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Existing warning behavior is unchanged across the whole package
yarn vitest --run --project storybookfrom2nd-gen/packages/swcConsole warnings still fire correctly in Storybook
yarn storybookfrom2nd-gen/packages/swc)variant,size,density,placement,orientation) via Controlsspectrum-web-components.adobe.com/?path=/docs/components-*--docs)Device review
Accessibility testing checklist
warnIf).