feat(validation): lifecycle placement of dev warnings - #6561
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…//github.com/adobe/spectrum-web-components into miwha/feat-dev-warning-remove-fallback-override
|
📚 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 |
Restore Tabs, AlertBanner, and Meter to the PR 2 (components) baseline so this PR carries only the philosophy-neutral work (lifecycle placement normalization + the validateEnum retrofit) and does not ship the contested coerce-to-default removal. The fallback-vs-warn-only decision is moving to its own ticket for team discussion; the removal lives on miwha/refactor-dev-warning-fallback-poc as a proof-of-concept to attach there. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 1st-gen renderIcon change (return `nothing` instead of empty templates) was committed to this 2nd-gen dev-warning branch by mistake. This branch should not touch 1st-gen files. Restore both 1st-gen AlertBanner files to the PR 2 baseline so the PR carries only 2nd-gen dev-warning placement work. The original change is preserved in commit 2603a2a if it is wanted on its own branch later. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…//github.com/adobe/spectrum-web-components into miwha/feat-dev-warning-refactors
…//github.com/adobe/spectrum-web-components into miwha/feat-dev-warning-refactors
rubencarvalho
left a comment
There was a problem hiding this comment.
Looks good to me! Just one little change request 😄
| } | ||
| } | ||
|
|
||
| if (changes.has('staticColor') && this.staticColor !== undefined) { |
There was a problem hiding this comment.
I believe this is duplicating what is below (lines 241-248)
not requesting changes, commenting only
nikkimk
left a comment
There was a problem hiding this comment.
I left a comment for discussion that i'll drop into the thread. If we decide to change course, we can always merge this and change direction in a follow up PR.
| * class MyComponent extends SpectrumElement { | ||
| * private nameObserver = new AttributeObserverController( | ||
| * this, | ||
| * ['aria-label', 'aria-labelledby'], |
There was a problem hiding this comment.
Because of cross root ARIA issues, hosts shouldn't be using the aria-label and aria-labelledby attributes directly. Instead we typically have a label attribute that sets this. I'm wondering if, rather than creating a controller here, we align the API to have label and labelledby properties that set aria-label and aria-labelledby, even in components where the host has the role.
| @@ -0,0 +1,212 @@ | |||
| /** | |||
There was a problem hiding this comment.
Thank you for using a demo component to illustrate this instead of an SWC component. It provides a simple component for us to understand how to implement and it helps us isolate issues to the controller or component. <3
| super.updated(changedProperties); | ||
| if (changedProperties.has('variant')) { | ||
| const constructor = this.constructor as typeof TooltipBase; | ||
| validateEnum(this, { |
There was a problem hiding this comment.
Not a bug: I see Tooltip and ProgressCircle fold the validateEnum check in willUpdate while others do in update. Again, functionally both is running pre-render but its an inconsistency where we are trying to create a pattern.
There was a problem hiding this comment.
Do we need this controller? It feels like a big overhead for what could (maybe) be simpler... e.g., the platform already gives us attributeChangedCallback/observedAttributes, which is what Lit uses internally to sync attributes to properties, and would get us attribute-change detection without a MutationObserver instance per host (even though I only see it being used on Dropzone for now? - which may also hint at some premature optimization).
But even setting that aside (and following up our Slack thread), we already have a first-party pattern for this, using accessible-label / accessibleLabel. If we lean on that convention instead of watching aria-label directly, we also won't need the controller for this case.
Aaaand as a bit of a tangent, but what I think we haven't decided / established a pattern is for labelledby. We've been treating aria-labelledby as the attribute... Should we extend the accessible-label pattern with an accessible-labelledby counterpart too?
Description
Moves pure property-value dev-warnings (
validateEnum, plus StatusLight's deprecation warnings) fromupdated()(post-render) to the pre-render phase, so a warning about an invalid value fires before first paint instead of after. For seven components: Asset, Card, IllustratedMessage, ProgressCircle, StatusLight, Tooltip, ActionButton.No behavior change. Only the timing of dev-mode warnings changes; rendered output, ARIA, and focus are untouched, and production is unaffected (dev warnings are stripped there). DOM-dependent checks stay in
updated()by design (Cardtitle-as-link/checkActionsSupport, ProgressCircle aria-label default).The separate, contested fallback/coercion change is intentionally excluded and lives as a do-not-merge POC in #6564.
Dev-warning revalidation
Added an
AttributeObserverControllerso accessibility warnings that readaria-label/aria-labelledbyre-check when those change. Before, the check only ran once when the component connected, so removing a label afterward went unnoticed. It only runs in dev, so production is unaffected.Also removed the
_hasWarnedflags from Dropzone and LinearProgressMixin. They looked like they re-showed a warning after fixing and re-breaking something, but the warning system already shows each message only once per session, so the flags weren't actually doing anything. Replaced them with a simple early return.Motivation and context
A property-value check in
updated()runs after the component has already rendered once with the invalid value. Running it pre-render surfaces the warning before paint and matches the style guide (validateEnuminupdate()beforesuper.update()).That pre-render approach works for reactive properties, but not for accessible names that live in
aria-label/aria-labelledby: those aren't reactive, so changing them never triggers a render and the check goes stale after the first run.AttributeObserverControllercovers that case by observing those attributes and requesting a re-check when they change (dev-only, so production is unaffected).While doing this, we also found the
_hasWarnedflags in Dropzone and LinearProgressMixin weren't doing anything: the warning system already dedups identical messages once per session, so the flags never re-surfaced a warning even though the code read as if they did. They've been removed in favor of a simple early return.Related issue(s)
Screenshots (if appropriate)
N/A, no visual change.
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
variant="banana".main.Device review
Accessibility testing checklist
Timing-only change; no roles, names, states, focus, or keyboard behavior are affected. Testing confirms no regression.
main.main(including Progress Circle's default "Loading").