Skip to content

Commit e6093dd

Browse files
committed
fix(forms): proper property and attribute reflection for button mixin
- Changed `HTMLElement` to `Element` for `commandForElement`, `interestForElement`, and `popoverTargetElement` in various controllers and mixins to enhance type flexibility. - Added new tests to verify behavior when `popovertarget` changes in `InterestInvokerController`. - Updated tests in `PopoverTriggerController` and `ButtonFormControlMixin` to reflect changes in element type handling. Signed-off-by: Cory Rylan <crylan@nvidia.com>
1 parent efb7a71 commit e6093dd

8 files changed

Lines changed: 241 additions & 101 deletions

projects/core/src/internal/button-form-control-usage.test.ts renamed to projects/core/src/forms/button-form-control-usage.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ describe('ButtonFormControlMixin core usage', () => {
9999
expect(button.tabIndex).toBe(-1);
100100

101101
button.disabled=false;
102-
button.readonly=true;
102+
button.readOnly=true;
103103
awaitelementIsStable(button);
104104
expect(button.readOnly).toBe(true);
105105
expect(button.hasAttribute('readonly')).toBe(true);

‎projects/forms/src/internal/controllers/type-interest-invoker.controller.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,55 @@ describe('InterestInvokerController', () => {
159159
expect(element.interestForElement).toBe(target);
160160
});
161161

162+
it('should re-resolve inferred targets when popovertarget changes',async()=>{
163+
fixture=awaitcreateFixture(html`
164+
<interest-invoker-controller-test-elementpopovertarget="first"></interest-invoker-controller-test-element>
165+
<divid="first" popover="hint"></div>
166+
<divid="second" popover="hint"></div>
167+
`);
168+
constelement=fixture.querySelector<InterestInvokerControllerTestElement>(
169+
'interest-invoker-controller-test-element'
170+
)!;
171+
constfirst=fixture.querySelector<HTMLElement>('#first')!;
172+
constsecond=fixture.querySelector<HTMLElement>('#second')!;
173+
constfirstDispatch=vi.spyOn(first,'dispatchEvent');
174+
constsecondDispatch=vi.spyOn(second,'dispatchEvent');
175+
176+
element.dispatchEvent(newMouseEvent('mouseenter'));
177+
element.setAttribute('popovertarget','second');
178+
element.dispatchEvent(newMouseEvent('mouseenter'));
179+
element.dispatchEvent(newMouseEvent('mouseleave'));
180+
181+
expect(firstDispatch.mock.calls.map(([event])=>event.type)).toEqual(['interest']);
182+
expect(secondDispatch.mock.calls.map(([event])=>event.type)).toEqual(['interest','loseinterest']);
183+
expect(element.interestForElement).toBe(second);
184+
});
185+
186+
it('should preserve explicitly assigned interest targets when popovertarget changes',async()=>{
187+
fixture=awaitcreateFixture(html`
188+
<interest-invoker-controller-test-elementpopovertarget="first"></interest-invoker-controller-test-element>
189+
<divid="first" popover="hint"></div>
190+
<divid="second" popover="hint"></div>
191+
<divid="explicit"></div>
192+
`);
193+
constelement=fixture.querySelector<InterestInvokerControllerTestElement>(
194+
'interest-invoker-controller-test-element'
195+
)!;
196+
constfirst=fixture.querySelector<HTMLElement>('#first')!;
197+
constexplicit=fixture.querySelector<HTMLElement>('#explicit')!;
198+
199+
element.dispatchEvent(newMouseEvent('mouseenter'));
200+
expect(element.interestForElement).toBe(first);
201+
202+
constexplicitInterest=untilEvent<InterestTestEvent>(explicit,'interest');
203+
element.interestForElement=explicit;
204+
element.setAttribute('popovertarget','second');
205+
element.dispatchEvent(newMouseEvent('mouseenter'));
206+
207+
expect((awaitexplicitInterest).source).toBe(element);
208+
expect(element.interestForElement).toBe(explicit);
209+
});
210+
162211
it('should no-op for missing targets and after disconnect',async()=>{
163212
fixture=awaitcreateFixture(
164213
html`<interest-invoker-controller-test-elementinterestfor="missing"></interest-invoker-controller-test-element>`

‎projects/forms/src/internal/controllers/type-interest-invoker.controller.ts‎

Lines changed: 55 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ type InterestInvokerHost = ReactiveElement & {
1212
};
1313

1414
exportclassTypeInterestInvokerController<TextendsInterestInvokerHost>implementsReactiveController{
15+
#inferredInterestForElement: HTMLElement|null=null;
16+
1517
constructor(privatehost: T){
1618
this.host.addController(this);
1719
}
@@ -39,36 +41,66 @@ export class TypeInterestInvokerController<T extends InterestInvokerHost> implem
3941
return;
4042
}
4143

42-
this.#updateInterestForElement();
43-
if(this.host.interestForElement){
44-
constinterest=newEvent('interest',{cancelable: true})asInterestEvent;
45-
interest.source=this.host;
46-
this.host.interestForElement.dispatchEvent(interest);
47-
}
44+
this.#dispatchInterestEvent('interest');
4845
};
4946

50-
#onLoseInterest =()=>{
51-
this.#updateInterestForElement();
52-
if(this.host.interestForElement){
53-
constevent=newEvent('loseinterest',{cancelable: true})asInterestEvent;
54-
event.source=this.host;
55-
this.host.interestForElement.dispatchEvent(event);
47+
#onLoseInterest =()=>this.#dispatchInterestEvent('loseinterest');
48+
49+
#dispatchInterestEvent(type: 'interest'|'loseinterest'){
50+
consttarget=this.#getInterestForElement();
51+
if(!target){
52+
return;
5653
}
57-
};
5854

59-
#updateInterestForElement(){
55+
constevent=newEvent(type,{cancelable: true})asInterestEvent;
56+
event.source=this.host;
57+
target.dispatchEvent(event);
58+
}
59+
60+
#getInterestForElement(){
61+
constexplicitTarget=this.#getExplicitInterestForElement();
62+
if(explicitTarget!==undefined){
63+
returnexplicitTarget;
64+
}
65+
66+
constinferredTarget=this.#getHintPopoverTarget();
67+
constpreviousTarget=this.#inferredInterestForElement;
68+
this.#inferredInterestForElement =inferredTarget;
69+
if(inferredTarget&&this.host.interestForElement!==inferredTarget){
70+
this.host.interestForElement=inferredTarget;
71+
}elseif(!inferredTarget&&previousTarget&&this.host.interestForElement===previousTarget){
72+
this.host.interestForElement=null;
73+
}
74+
returninferredTarget;
75+
}
76+
77+
#getExplicitInterestForElement(): HTMLElement|null|undefined{
6078
constinterestForIdRef=this.host.getAttribute('interestfor');
61-
if(interestForIdRef&&!this.host.interestForElement){
62-
this.host.interestForElement=
63-
getFlattenedDOMTree(this.host.getRootNode()).find(element=>element.id===interestForIdRef)??null;
79+
if(interestForIdRef){
80+
this.#inferredInterestForElement =null;
81+
returnthis.#getElementById(interestForIdRef);
6482
}
6583

66-
constpopoverTargetIdRef=this.host.getAttribute('popovertarget');
67-
if(popoverTargetIdRef&&!interestForIdRef){
68-
consttarget=getFlattenedDOMTree(this.host.getRootNode()).find(element=>element.id===popoverTargetIdRef);
69-
if(target&&target.popover==='hint'){
70-
this.host.interestForElement=target;
71-
}
84+
constinterestForElement=this.host.interestForElement;
85+
if(!interestForElement||interestForElement===this.#inferredInterestForElement){
86+
returnundefined;
7287
}
88+
89+
this.#inferredInterestForElement =null;
90+
returninterestForElement;
91+
}
92+
93+
#getHintPopoverTarget(){
94+
constpopoverTarget=this.host.popoverTargetElement;
95+
if(popoverTargetinstanceofHTMLElement&&popoverTarget.popover==='hint'){
96+
returnpopoverTarget;
97+
}
98+
99+
consttarget=this.#getElementById(this.host.getAttribute('popovertarget'));
100+
returntargetinstanceofHTMLElement&&target.popover==='hint' ? target : null;
101+
}
102+
103+
#getElementById(id: string|null){
104+
returnid ? (getFlattenedDOMTree(this.host.getRootNode()).find(element=>element.id===id)??null) : null;
73105
}
74106
}

‎projects/forms/src/internal/controllers/type-popover-trigger.controller.test.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ class PopoverTriggerControllerTestElement extends HTMLElement {
1414
interestForElement: HTMLElement|null=null;
1515
popoverTargetAction?: PopoverTargetAction;
1616
popoverTargetElement: HTMLElement|null=null;
17-
popovertarget?: string;
1817
#controllers =newSet<ReactiveController>();
1918

2019
constructor(){
@@ -58,7 +57,7 @@ describe('PopoverTriggerController', () => {
5857
constpopover=fixture.querySelector<HTMLElement>('[popover]')!;
5958
consttogglePopover=vi.spyOn(popover,'togglePopover').mockImplementation(()=>false);
6059

61-
element.popovertarget='popover';
60+
element.setAttribute('popovertarget','popover');
6261
awaitemulateClick(element);
6362

6463
expect(element.popoverTargetElement).toBe(popover);
@@ -101,12 +100,12 @@ describe('PopoverTriggerController', () => {
101100

102101
element.interestForElement=tooltip;
103102
element.popoverTargetAction='show';
104-
element.popovertarget='first';
103+
element.setAttribute('popovertarget','first');
105104
awaitemulateClick(element);
106105
first.hidePopover();
107106

108107
loseInterest.mockClear();
109-
element.popovertarget='second';
108+
element.setAttribute('popovertarget','second');
110109
awaitemulateClick(element);
111110

112111
expect(element.popoverTargetElement).toBe(second);
@@ -249,7 +248,7 @@ describe('PopoverTriggerController', () => {
249248

250249
Object.defineProperty(popover,'anchor',{configurable: true,value: 'anchor'});
251250
element.interestForElement=tooltip;
252-
element.popovertarget='popover';
251+
element.setAttribute('popovertarget','popover');
253252
awaitemulateClick(element);
254253

255254
expect(element.popoverTargetElement).toBe(popover);

‎projects/forms/src/internal/controllers/type-popover-trigger.controller.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ type PopoverTriggerHost = ReactiveElement & {
1313
interestForElement: HTMLElement|null;
1414
popoverTargetAction?: PopoverTargetAction;
1515
popoverTargetElement: HTMLElement|null;
16-
popovertarget?: string;
1716
};
1817

1918
exportclassTypePopoverTriggerController<TextendsPopoverTriggerHost>implementsReactiveController{
@@ -35,7 +34,7 @@ export class TypePopoverTriggerController<T extends PopoverTriggerHost> implemen
3534
constpopoverTargetElement=this.host.popoverTargetElement;
3635
constcontrollerResolvedTarget=popoverTargetElement===this.#resolvedPopoverTarget;
3736

38-
if(!this.host.popovertarget){
37+
if(!this.host.getAttribute('popovertarget')){
3938
this.#resolvedPopoverTarget =undefined;
4039
if(controllerResolvedTarget){
4140
this.host.popoverTargetElement=null;
@@ -50,7 +49,9 @@ export class TypePopoverTriggerController<T extends PopoverTriggerHost> implemen
5049
}
5150

5251
constresolvedPopoverTarget=
53-
getFlattenedDOMTree(this.host.getRootNode()).find(element=>element.id===this.host.popovertarget)??null;
52+
getFlattenedDOMTree(this.host.getRootNode()).find(
53+
element=>element.id===this.host.getAttribute('popovertarget')
54+
)??null;
5455
if(resolvedPopoverTarget!==popoverTargetElement){
5556
this.host.popoverTargetElement=resolvedPopoverTarget;
5657
}

‎projects/forms/src/mixins/button.test.ts‎

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -552,24 +552,15 @@ describe('ButtonFormControlMixin', () => {
552552

553553
it('should sync removable string attributes',async()=>{
554554
submitButton.value='saved';
555-
submitButton.popovertarget='popover';
556-
submitButton.commandfor='command-target';
557555
submitButton.command='--test';
558-
submitButton.interestfor='interest-target';
559556
awaitelementIsStable(submitButton);
560557

561558
submitButton.removeAttribute('value');
562-
submitButton.removeAttribute('popovertarget');
563-
submitButton.removeAttribute('commandfor');
564559
submitButton.removeAttribute('command');
565-
submitButton.removeAttribute('interestfor');
566560
awaitelementIsStable(submitButton);
567561

568562
expect(submitButton.value).toBeUndefined();
569-
expect(submitButton.popovertarget).toBeUndefined();
570-
expect(submitButton.commandfor).toBe(null);
571563
expect(submitButton.command).toBeUndefined();
572-
expect(submitButton.interestfor).toBe(null);
573564
});
574565
});
575566

@@ -751,6 +742,69 @@ describe('ButtonFormControlMixin', () => {
751742
expect((propertyCommand.mock.calls[0]?.[0]asCommandTestEvent).source).toBe(button);
752743
});
753744

745+
it('should reflect native target properties to their corresponding attributes',async()=>{
746+
fixture=awaitcreateFixture(html`
747+
<button-form-control-mixin-test-element
748+
commandfor="attribute-command"
749+
interestfor="attribute-interest"
750+
popovertarget="attribute-popover"
751+
popovertargetaction="show"
752+
></button-form-control-mixin-test-element>
753+
<divid="attribute-command"></div>
754+
<divid="attribute-interest"></div>
755+
<divid="attribute-popover" popover></div>
756+
<divid="property-command"></div>
757+
<divid="property-interest"></div>
758+
<divid="property-popover" popover></div>
759+
`);
760+
button=fixture.querySelector<ButtonFormControlMixinTestElement>('button-form-control-mixin-test-element')!;
761+
constattributeCommand=fixture.querySelector<HTMLElement>('#attribute-command')!;
762+
constattributeInterest=fixture.querySelector<HTMLElement>('#attribute-interest')!;
763+
constattributePopover=fixture.querySelector<HTMLElement>('#attribute-popover')!;
764+
constpropertyCommand=fixture.querySelector<HTMLElement>('#property-command')!;
765+
constpropertyInterest=fixture.querySelector<HTMLElement>('#property-interest')!;
766+
constpropertyPopover=fixture.querySelector<HTMLElement>('#property-popover')!;
767+
awaitelementIsStable(button);
768+
769+
expect(button.commandForElement).toBe(attributeCommand);
770+
expect(button.interestForElement).toBe(attributeInterest);
771+
expect(button.popoverTargetElement).toBe(attributePopover);
772+
expect(button.popoverTargetAction).toBe('show');
773+
774+
button.commandForElement=propertyCommand;
775+
button.interestForElement=propertyInterest;
776+
button.popoverTargetElement=propertyPopover;
777+
button.popoverTargetAction='hide';
778+
awaitelementIsStable(button);
779+
780+
expect(button.getAttribute('commandfor')).toBe('');
781+
expect(button.getAttribute('interestfor')).toBe('');
782+
expect(button.getAttribute('popovertarget')).toBe('');
783+
expect(button.getAttribute('popovertargetaction')).toBe('hide');
784+
expect(button.commandForElement).toBe(propertyCommand);
785+
expect(button.interestForElement).toBe(propertyInterest);
786+
expect(button.popoverTargetElement).toBe(propertyPopover);
787+
788+
button.setAttribute('commandfor','attribute-command');
789+
button.setAttribute('interestfor','attribute-interest');
790+
button.setAttribute('popovertarget','attribute-popover');
791+
button.setAttribute('popovertargetaction','invalid');
792+
awaitelementIsStable(button);
793+
794+
expect(button.commandForElement).toBe(attributeCommand);
795+
expect(button.interestForElement).toBe(attributeInterest);
796+
expect(button.popoverTargetElement).toBe(attributePopover);
797+
expect(button.popoverTargetAction).toBe('toggle');
798+
799+
button.commandForElement=null;
800+
button.interestForElement=null;
801+
button.popoverTargetElement=null;
802+
803+
expect(button.hasAttribute('commandfor')).toBe(false);
804+
expect(button.hasAttribute('interestfor')).toBe(false);
805+
expect(button.hasAttribute('popovertarget')).toBe(false);
806+
});
807+
754808
it('should no-op command dispatch when clicks are default-prevented, disabled, readonly, or commandless',async()=>{
755809
fixture=awaitcreateFixture(html`
756810
<button-form-control-mixin-test-elementcommand="--test" commandfor="target"></button-form-control-mixin-test-element>
@@ -860,7 +914,7 @@ describe('ButtonFormControlMixin', () => {
860914
emulateClick(button);
861915
first.hidePopover();
862916

863-
button.popovertarget='second';
917+
button.setAttribute('popovertarget','second');
864918
awaitelementIsStable(button);
865919
emulateClick(button);
866920

0 commit comments

Comments
 (0)