feat(web-components): add Dropdown, Listbox, and Option components #32116
Conversation
📊 Bundle size report✅ No changes found |
|
@radium-v I noticed something interesting trying to use this in a dialog. The dropdown's beforetoggle handler never gets called for some reason. I'm not sure why this happens since the popover API supports nested popover elements |
radium-v
force-pushed
the
users/radium-v/wc-dropdown
branch
from
e11d0b1 to
5c1d661
Compare
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentuiv8 Visual Regression Report
Callout 5 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Callout.Left top edge.chromium.png | 1949 | Changed |
| Callout.Beak 25.chromium.png | 2186 | Changed |
| Callout.Bottom left edge.chromium.png | 2309 | Changed |
| Callout.Right top edge.chromium.png | 1127 | Changed |
| Callout.No callout width specified.chromium.png | 2319 | Changed |
react-charting-AreaChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| react-charting-AreaChart.Custom Accessibility.chromium.png | 11 | Changed |
🕵 FluentUIV0 No visual regressions between this PR and main |
change/@fluentui-web-components-7b17a2b2-f134-4934-8331-709bea980b6c.json
Show resolved
Hide resolved
radium-v
force-pushed
the
users/radium-v/wc-dropdown
branch
from
5c1d661 to
cb1a6c9
Compare
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
| ${ref('popoverContainer')} | ||
| > | ||
| <slot ${ref('listboxSlot')}></slot> | ||
| </div> |
There was a problem hiding this comment.
With this DOM structure, you may be able to get away with not using the CSS Anchor Positioning polyfill at all, because the listbox/popover would be positioned correctly at its natural flow position, it won't flip or change size based on the trigger, but that might be an acceptable downgrade.
radium-v
force-pushed
the
users/radium-v/wc-dropdown
branch
from
cb1a6c9 to
e74eb0b
Compare
radium-v
force-pushed
the
users/radium-v/wc-dropdown
branch
2 times, most recently
from
134af2c to
e79a83e
Compare
radium-v
changed the title
radium-v
changed the title
radium-v
force-pushed
the
users/radium-v/wc-dropdown
branch
from
99050ec to
3761ed8
Compare
| @@ -1,7 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentui-web-components-v3 Visual Regression Report
Avatar 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Avatar.Image.chromium.png | 248 | Changed |
| @@ -1,7 +1,8 @@ | |||
| { | |||
| "extends": "../../tsconfig.base.wc.json", | |||
| "compilerOptions": { | |||
| "target": "ES2019", | |||
| "target": "ES2022", | |||
| "useDefineForClassFields": false, | |||
There was a problem hiding this comment.
can we move these to tsconfig.base.wc.json ?
There was a problem hiding this comment.
with these added t obase we can remove it per project configs right ?
| * An Option Custom HTML Element. | ||
| * Implements the {@link https://w3c.github.io/aria/#option | ARIA option } role. | ||
| * | ||
| * @slot checked-indicator - The checked indicator |
There was a problem hiding this comment.
Missing the default slot, description slot, and the start slot.
| * @slot checked-indicator - The checked indicator | ||
| * @slot indeterminate-indicator - The indeterminate indicator | ||
| * @fires change - Emits a custom change event when the checked state changes | ||
| * @fires input - Emits a custom input event when the checked state changes |
There was a problem hiding this comment.
It doesn't seem these 2 events are being emitted.
|
|
||
| /** | ||
| * A Dropdown Custom HTML Element. | ||
| * Implements the {@link https://w3c.github.io/aria/#combobox | ARIA combobox } role. |
There was a problem hiding this comment.
Missing documentation for slots.
| * @remarks | ||
| * HTML Attribute: `id` | ||
| */ | ||
| @attr({ attribute: 'id' }) |
There was a problem hiding this comment.
Does this need mode: 'fromView'?
There was a problem hiding this comment.
Couple legitimate change requests on the big boy Dropdown.ts, but mostly just notes to leave comments on why we're diverting from the norm on a couple occasions.
Overall great work @radium-v. Very clean and pretty code. It was pleasant to read through.
| // Warning: (ae-forgotten-export) The symbol "Start" needs to be exported by the entry point index.d.ts | ||
| // | ||
| // @public | ||
| class Option_2 extends FASTElement implements Start { |
There was a problem hiding this comment.
Option_2 seems wrong here. Maybe we need to re-run API docs?
| export const dropdownInputTemplate = html<BaseDropdown>` | ||
| <input | ||
| @input="${(x, c) => x.inputHandler(c.event as InputEvent)}" | ||
| @change="${(x, c) => x.changeHandler(c.event as InputEvent)}" | ||
| aria-activedescendant="${x => x.activeDescendant}" | ||
| aria-controls="${x => x.listbox?.id ?? null}" | ||
| aria-labelledby="${x => x.ariaLabelledBy}" | ||
| aria-expanded="${x => x.open}" | ||
| aria-haspopup="listbox" | ||
| placeholder="${x => x.placeholder}" | ||
| role="combobox" | ||
| ?disabled="${x => x.disabled}" | ||
| type="${x => x.type}" | ||
| value="${x => x.valueAttribute}" | ||
| ${ref('control')} | ||
| /> | ||
| `; | ||
|
|
||
| export const dropdownButtonTemplate = html<BaseDropdown>` | ||
| <button | ||
| aria-activedescendant="${x => x.activeDescendant}" | ||
| aria-controls="${x => x.listbox?.id ?? null}" | ||
| aria-expanded="${x => x.open}" | ||
| aria-haspopup="listbox" | ||
| role="combobox" | ||
| ?disabled="${x => x.disabled}" | ||
| ${ref('control')} | ||
| > | ||
| ${x => x.displayValue} | ||
| </button> | ||
| `; |
There was a problem hiding this comment.
Because these both had ref('control') and role="combobox" I was a little bit confused on how they're being used in the same template. I see later in BaseDropdown#insertControl that we conditionally insert these based on if it's a DropdownType.combobox. We could rename dropdownInputTemplate to comboboxInputTemplate to disambiguate but maybe the cleanest solution is to leave a comment for future us on why both of these exist.
Re: insertControl - this also seems like a good thing to document why we're using a slightly different approach to inject templates instead of something like a when. I see that method wipes out any slotted controls and injects either one of these templates, but it injects it into the root and not the slot?
I think I'm confused about how that's intended to work. If insertControl is auto-called by typeChanged and connectedCallback can a consumer supply their own dropdown templates without forking the base class?
There was a problem hiding this comment.
Because these both had
ref('control')androle="combobox"I was a little bit confused on how they're being used in the same template. I see later inBaseDropdown#insertControlthat we conditionally insert these based on if it's aDropdownType.combobox. We could renamedropdownInputTemplatetocomboboxInputTemplateto disambiguate but maybe the cleanest solution is to leave a comment for future us on why both of these exist.
Documentation comments incoming!
Re:
insertControl- this also seems like a good thing to document why we're using a slightly different approach to inject templates instead of something like awhen. I see that method wipes out any slotted controls and injects either one of these templates, but it injects it into the root and not the slot?
The input is inserted into the light dom, and assigned to the control slot. It's done in conjunction with slotAssignment: "manual" which is set in the element definition. This is 100% to sidestep cross-root ARIA limitations. Using when would place the input into the shadow dom, which would break ARIA. I'll make sure to add documentation so that this is clearly explained.
I think I'm confused about how that's intended to work. If
insertControlis auto-called bytypeChangedandconnectedCallbackcan a consumer supply their own dropdown templates without forking the base class?
This is a great question. I'll investigate ways to possibly roll both of the templates into the main one, so that they're not pulled in directly.
| const rect = this.getBoundingClientRect(); | ||
| if (rect.top < window.innerHeight - rect.bottom) { |
There was a problem hiding this comment.
getBoundingClientRect() and window.innerHeight both trigger a forced layout reflow which could have some perf problems inside a window.onscroll handler. At a minimum we'd want to debounce this. At a maximum, we'd want to figure out an alternative? like max-height: 40vh or something?
| const observerCallback = (entries: IntersectionObserverEntry[]): void => { | ||
| entries.forEach(entry => { | ||
| const target = entry.target as Listbox; | ||
| if (isListbox(target)) { | ||
| if (inRange(entry.intersectionRatio, 0, 1)) { | ||
| toggleState( | ||
| target.dropdown?.elementInternals, | ||
| 'flip-block', | ||
| entry.intersectionRect.bottom >= window.innerHeight, | ||
| ); | ||
| } | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| Dropdown.AnchorPositionFallbackObserver = | ||
| Dropdown.AnchorPositionFallbackObserver ?? new IntersectionObserver(observerCallback, { threshold: [0, 1] }); |
There was a problem hiding this comment.
Some comments on why we needed to setup this intersection observer would be nice.
|
|
||
| Dropdown.AnchorPositionFallbackStyleElements.set(this, this.anchorPositionFallbackStyleElement); | ||
|
|
||
| Dropdown.AnchorPositionFallbackTimeout = requestIdleCallback( |
There was a problem hiding this comment.
Safari doesn't have requestIdleCallback yet 😭 only in Technical Preview as of now. Maybe just use requestAnimationFrame or setTimeout?
There was a problem hiding this comment.
It's ponyfilled in packages/web-components/src/utils/idle-callback.ts
microsoft-github-policy-service
bot
added
Needs: Author Feedback
and removed
Needs: Author Feedback
labels
New Behavior
Adds
<fluent-dropdown>,<fluent-listbox>and<fluent-option>components.Todo
smallsizelargesizetransparentappearancefilled-lighterappearancefilled-darkerappearanceRelated Issue(s)