Skip to content

Commit

Permalink
Change popover invoking attributes
Browse files Browse the repository at this point in the history 
See [1] for discussion. This changes the Popover invoking attribute
behavior. Previously, three attributes were used to set the invoking
relationship and behavior:
 - popovertoggletarget
 - popovershowtarget
 - popoverhidetarget

Those each could point to an idref for a popover, and the behavior
would be controlled by which attribute was used. However, there was
a confusing situation in the case that multiple such attributes
were used on a single element, and even more so when they pointed
to different elements. While there was concrete logic for resolving
that situation, it was confusing at best.

The new logic is controlled by two attributes:
 - popovertarget
 - popovertargetaction

The element reference is dictated by `popovertarget`, via an idref
or through element reflection (via `foo.popoverTargetElement = bar`).
The behavior is dictated by the (string valued) popovertargetaction
attribute, which can be one of "toggle", "show", or "hide". If any
other value is used (including missing attribute or invalid value),
the behavior and IDL reflected value is "toggle".

[1] whatwg/html#8894 (comment)

Bug: 1307772
Change-Id: I2e530efe26032599f9376c8d5f4fe2a7f430a26b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4288730
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1109867}
  • Loading branch information
@mfreed7 @chromium-wpt-export-bot
mfreed7 authored and chromium-wpt-export-bot committed Feb 24, 2023
1 parent 879ec5b commit 40dbd3d
Show file tree
Hide file tree
Showing 9 changed files with 119 additions and 147 deletions.
2 changes: 1 addition & 1 deletion html/semantics/popovers/popover-anchor-idl-property.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<button id=b1>This is an anchor button</button>
<div popover id=p1 anchor=b1>This is a popover</div>
<button id=b2 popovertoggletarget=p1>This button invokes the popover but isn't an anchor</button>
<button id=b2 popovertarget=p1>This button invokes the popover but isn't an anchor</button>

<script>
test(function() {
Expand Down
4 changes: 2 additions & 2 deletions html/semantics/popovers/popover-anchor-nested-display.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
<link rel=match href="popover-anchor-nested-display-ref.html">

<button id=main-menu-button popovertoggletarget=main-menu>Show menu</button>
<button id=main-menu-button popovertarget=main-menu>Show menu</button>

<div id=main-menu popover anchor=main-menu-button>
<div>Foo</div>
<button id=nested-menu-button popovertoggletarget=nested-menu>
<button id=nested-menu-button popovertarget=nested-menu>
Show nested menu
</button>
<div>Bar</div>
Expand Down
20 changes: 10 additions & 10 deletions html/semantics/popovers/popover-focus-2.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@
<button id=button1>Button1</button>
<div popover id=popover1 style="top:100px">
<button id=inside_popover1>Inside1</button>
<button id=invoker2 popovertoggletarget=popover2>Nested Invoker 2</button>
<button id=invoker2 popovertarget=popover2>Nested Invoker 2</button>
<button id=inside_popover2>Inside2</button>
</div>
<button id=button2>Button2</button>
<button popovertoggletarget=popover1 id=invoker1>Invoker1</button>
<button popovertarget=popover1 id=invoker1>Invoker1</button>
<button id=button3>Button3</button>
<div popover id=popover2 style="top:200px">
<button id=inside_popover3>Inside3</button>
<button id=invoker3 popovertoggletarget=popover3>Nested Invoker 3</button>
<button id=invoker3 popovertarget=popover3>Nested Invoker 3</button>
</div>
<div popover id=popover3 style="top:300px">
Non-focusable popover
Expand Down Expand Up @@ -90,11 +90,11 @@
}, "Popover focus navigation");
</script>

<button id=circular0 popovertoggletarget=popover4>Invoker</button>
<button id=circular0 popovertarget=popover4>Invoker</button>
<div id=popover4 popover>
<button id=circular1 autofocus popoverhidetarget=popover4></button>
<button id=circular2 popovershowtarget=popover4></button>
<button id=circular3 popovertoggletarget=popover4></button>
<button id=circular1 autofocus popovertarget=popover4 popovertargetaction=hide></button>
<button id=circular2 popovertarget=popover4 popovertargetaction=show></button>
<button id=circular3 popovertarget=popover4></button>
</div>
<button id=circular4>after</button>
<script>
Expand All @@ -107,16 +107,16 @@
</script>

<div id=deleted>
<button popovershowtarget=deleted1>Show popover</button>
<button popovertarget=deleted1 popovertargetaction=show>Show popover</button>
<div popover id=deleted1>
<button popoverhidetarget=deleted1 autofocus>Hide popover</button>
<button popovertarget=deleted1 popovertargetaction=hide autofocus>Hide popover</button>
</div>
</div>
<script>
promise_test(async t => {
const invoker = document.querySelector('#deleted>button');
const popover = document.querySelector('#deleted>[popover]');
const hideButton = popover.querySelector('[popoverhidetarget]');
const hideButton = popover.querySelector('[popovertargetaction=hide]');
invoker.focus(); // Make sure button is focused.
assert_equals(document.activeElement,invoker);
await sendEnter(); // Activate the invoker
Expand Down
10 changes: 5 additions & 5 deletions html/semantics/popovers/popover-focus.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
button.remove();
});
popover.id = popoverId;
button.setAttribute('popovertoggletarget', popoverId);
button.setAttribute('popovertarget', popoverId);
return button;
}
function addPriorFocus(t) {
Expand Down Expand Up @@ -187,7 +187,7 @@
const priorFocus = addPriorFocus(t);
assert_false(popover.matches(':open'), 'popover should start out hidden');
let button = addInvoker(t, popover);
assert_equals(button.getAttribute('popovertoggletarget'), popover.id, 'This test assumes the button uses `popovertoggletarget`.');
assert_equals(button.getAttribute('popovertarget'), popover.id, 'This test assumes the button uses `popovertarget`.');
assert_not_equals(button, priorFocus, 'Stranger things have happened');
assert_false(popover.contains(button), 'Start with a non-contained button');
priorFocus.focus();
Expand All @@ -200,8 +200,8 @@
assert_false(isElementVisible(popover));

// Same thing, but the button is contained within the popover
button.removeAttribute('popovertoggletarget');
button.setAttribute('popoverhidetarget', popover.id);
button.setAttribute('popovertarget', popover.id);
button.setAttribute('popovertargetaction', 'hide');
popover.appendChild(button);
t.add_cleanup(() => button.remove());
priorFocus.focus();
Expand All @@ -216,7 +216,7 @@
assert_equals(document.activeElement, priorFocus, 'Contained button should return focus to the previously focused element');
assert_false(isElementVisible(popover));

// Same thing, but the button is unrelated (no popovertoggletarget)
// Same thing, but the button is unrelated (no popovertarget)
button = document.createElement('button');
document.body.appendChild(button);
priorFocus.focus();
Expand Down
179 changes: 76 additions & 103 deletions html/semantics/popovers/popover-invoking-attribute.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,15 @@

<body>
<script>
const buttonLogic = (t,s,h) => {
// This mimics the expected logic for button invokers:
let expectedBehavior = t ? "toggle" : (s ? "show" : (h ? "hide" : "none"));
let expectedId = t || s || h || 1;
if (!t && s && h) {
// Special case - only use toggle if the show/hide idrefs match.
expectedBehavior = (s === h) ? "toggle" : "show";
const actionReflectionLogic = (action) => {
switch (action?.toLowerCase()) {
case "show": return "show";
case "hide": return "hide";
default: return "toggle";
}
return {expectedBehavior, expectedId};
}
const noActivationLogic = (t,s,h) => {
// This does not activate any popovers.
return {expectedBehavior: "none", expectedId: 1};
const noActivationLogic = (action) => {
return "none";
}
function makeElementWithType(element,type) {
return (test) => {
Expand All @@ -35,26 +31,20 @@
return el;
};
}
function setInvokingContentAttribute(invoker,attr,idref) {
invoker.setAttribute(attr,idref);
assert_equals(invoker[attr + "Element"],document.getElementById(idref));
}
const supportedButtonTypes = ['button','reset','submit',''].map(type => {
return {
name: `<button type="${type}">`,
makeElement: makeElementWithType('button',type),
invokeFn: el => {el.focus(); el.click()},
getExpectedLogic: buttonLogic,
supported: true,
getExpectedLogic: actionReflectionLogic,
};
});
const supportedInputButtonTypes = ['button','reset','submit','image'].map(type => {
return {
name: `<input type="${type}">`,
makeElement: makeElementWithType('input',type),
invokeFn: el => {el.focus(); el.click()},
getExpectedLogic: buttonLogic,
supported: true,
getExpectedLogic: actionReflectionLogic,
};
});
const unsupportedTypes = ['text','email','password','search','tel','url','checkbox','radio','range','file','color','date','datetime-local','month','time','week','number'].map(type => {
Expand All @@ -63,7 +53,6 @@
makeElement: makeElementWithType('input',type),
invokeFn: (el) => {el.focus();},
getExpectedLogic: noActivationLogic, // None of these support popover invocation
supported: false,
};
});
const invokers = [
Expand All @@ -74,86 +63,70 @@
window.addEventListener('load', () => {
["auto","manual"].forEach(type => {
invokers.forEach(testcase => {
let t_set = [1], s_set = [1], h_set = [1];
if (testcase.supported) {
t_set = s_set = h_set = [0,1,2]; // Test all permutations
}
t_set.forEach(t => {
s_set.forEach(s => {
h_set.forEach(h => {
[false,true].forEach(use_idl => {
promise_test(async test => {
const popover1 = Object.assign(document.createElement('div'),{popover: type, id: 'popover-1'});
const popover2 = Object.assign(document.createElement('div'),{popover: type, id: 'popover-2'});
assert_equals(popover1.popover,type);
assert_equals(popover2.popover,type);
assert_not_equals(popover1.id,popover2.id);
const invoker = testcase.makeElement(test);
if (use_idl) {
invoker.popoverToggleTargetElement = t===1 ? popover1 : (t===2 ? popover2 : null);
invoker.popoverShowTargetElement = s===1 ? popover1 : (s===2 ? popover2 : null);
invoker.popoverHideTargetElement = h===1 ? popover1 : (h===2 ? popover2 : null);
} else {
if (t) setInvokingContentAttribute(invoker,'popoverToggleTarget',t===1 ? popover1.id : popover2.id);
if (s) setInvokingContentAttribute(invoker,'popoverShowTarget',s===1 ? popover1.id : popover2.id);
if (h) setInvokingContentAttribute(invoker,'popoverHideTarget',h===1 ? popover1.id : popover2.id);
}
assert_true(!document.getElementById(popover1.id));
assert_true(!document.getElementById(popover2.id));
document.body.appendChild(popover1);
document.body.appendChild(popover2);
test.add_cleanup(() => {
popover1.remove();
popover2.remove();
});
const {expectedBehavior, expectedId} = testcase.getExpectedLogic(t,s,h);
const otherId = expectedId !== 1 ? 1 : 2;
function assertPopoverShowing(num,state,message) {
assert_true(num>0,`Invalid expectedId ${num}`);
assert_equals((num===1 ? popover1 : popover2).matches(':open'),state,message || "");
}
assertPopoverShowing(expectedId,false);
assertPopoverShowing(otherId,false);
await testcase.invokeFn(invoker);
assert_equals(document.activeElement,invoker,'Focus should end up on the invoker');
assertPopoverShowing(otherId,false,'The other popover should never change');
switch (expectedBehavior) {
case "toggle":
case "show":
assertPopoverShowing(expectedId,true,'Toggle or show should show the popover');
(expectedId===1 ? popover1 : popover2).hidePopover(); // Hide the popover
break;
case "hide":
case "none":
assertPopoverShowing(expectedId,false,'Hide or none should leave the popover hidden');
break;
default:
assert_unreached();
}
if (expectedBehavior === "none") {
// If no behavior is expected, then there is nothing left to test. Even re-focusing
// a control that has no expected behavior may hide an open popover via light dismiss.
return;
}
(expectedId===1 ? popover1 : popover2).showPopover(); // Show the popover directly
assert_equals(document.activeElement,invoker,'The popover should not shift focus');
assertPopoverShowing(expectedId,true);
assertPopoverShowing(otherId,false);
await testcase.invokeFn(invoker);
assertPopoverShowing(otherId,false,'The other popover should never change');
switch (expectedBehavior) {
case "toggle":
case "hide":
assertPopoverShowing(expectedId,false,'Toggle or hide should hide the popover');
break;
case "show":
assertPopoverShowing(expectedId,true,'Show should leave the popover showing');
break;
default:
assert_unreached();
}
},`Test ${testcase.name}, t=${t}, s=${s}, h=${h}, ${use_idl ? "IDL" : "Content Attr"}, with popover=${type}`);
});
["toggle","hide","show","ShOw","garbage",null,undefined].forEach(action => {
[false,true].forEach(use_idl_for_target => {
[false,true].forEach(use_idl_for_action => {
promise_test(async test => {
const popover = Object.assign(document.createElement('div'),{popover: type, id: 'my-popover'});
assert_equals(popover.popover,type,'reflection');
const invoker = testcase.makeElement(test);
if (use_idl_for_target) {
invoker.popoverTargetElement = popover;
assert_equals(invoker.getAttribute('popovertarget'),'','attribute value');
} else {
invoker.setAttribute('popovertarget',popover.id);
}
if (use_idl_for_action) {
invoker.popoverTargetAction = action;
assert_equals(invoker.getAttribute('popovertargetaction'),String(action),'action reflection');
} else {
invoker.setAttribute('popovertargetaction',action);
}
assert_true(!document.getElementById(popover.id));
assert_equals(invoker.popoverTargetElement,null,'targetElement should be null before the popover is in the document');
assert_equals(invoker.popoverTargetAction,actionReflectionLogic(action),'action should be correct immediately');
document.body.appendChild(popover);
test.add_cleanup(() => {popover.remove();});
assert_equals(invoker.popoverTargetElement,popover,'target element should be returned once it\'s in the document');
assert_false(popover.matches(':open'));
await testcase.invokeFn(invoker);
assert_equals(document.activeElement,invoker,'Focus should end up on the invoker');
expectedBehavior = testcase.getExpectedLogic(action);
switch (expectedBehavior) {
case "toggle":
case "show":
assert_true(popover.matches(':open'),'Toggle or show should show the popover');
popover.hidePopover(); // Hide the popover
break;
case "hide":
case "none":
assert_false(popover.matches(':open'),'Hide or none should leave the popover hidden');
break;
default:
assert_unreached();
}
if (expectedBehavior === "none") {
// If no behavior is expected, then there is nothing left to test. Even re-focusing
// a control that has no expected behavior may hide an open popover via light dismiss.
return;
}
assert_false(popover.matches(':open'));
popover.showPopover(); // Show the popover directly
assert_equals(document.activeElement,invoker,'The popover should not shift focus');
assert_true(popover.matches(':open'));
await testcase.invokeFn(invoker);
switch (expectedBehavior) {
case "toggle":
case "hide":
assert_false(popover.matches(':open'),'Toggle or hide should hide the popover');
break;
case "show":
assert_true(popover.matches(':open'),'Show should leave the popover showing');
break;
default:
assert_unreached();
}
},`Test ${testcase.name}, action=${action}, ${use_idl_for_target ? "popoverTarget IDL" : "popovertarget attr"}, ${use_idl_for_action ? "popoverTargetAction IDL" : "popovertargetaction attr"}, with popover=${type}`);
});
});
});
Expand All @@ -164,7 +137,7 @@



<button popovertoggletarget=p1>Toggle Popover 1</button>
<button popovertarget=p1>Toggle Popover 1</button>
<div popover id=p1 style="border: 5px solid red;top: 100px;left: 100px;">This is popover #1</div>

<script>
Expand Down Expand Up @@ -206,7 +179,7 @@
await assertState(true,2,1);
popover.hidePopover();
await assertState(false,2,2);
}, "Clicking a popovertoggletarget button opens a closed popover (also check event counts)");
}, "Clicking a popovertarget button opens a closed popover (also check event counts)");

promise_test(async () => {
showCount = hideCount = 0;
Expand All @@ -215,6 +188,6 @@
await assertState(true,1,0);
await clickOn(button);
await assertState(false,1,1);
}, "Clicking a popovertoggletarget button closes an open popover (also check event counts)");
}, "Clicking a popovertarget button closes an open popover (also check event counts)");
});
</script>
Loading

0 comments on commit 40dbd3d

Please sign in to comment.