Add first draft of Popup (Editor's Draft)

[melanierichards]

This change adds an incubation spec for Popup (Editor's Draft). The text is based on designs from the initial popup proposal, modulo any previously-resolved issues from Open UI telecons (with the exception of #335, to be landed in a follow-up PR).

Notes:

• I have erred on the side of writing WHATWG-style spec text, and in some places have directly lifted that style of markup from the HTML spec (e.g. the frontmatter). Some areas probably need more technical detail. We can address either in comments or this PR or through new issues.
• Intent is for there to eventually be "CSS Incubation Text" and "HTML-AAM Incubation Text" sections, for those venues.
• I've added "Editors" front matter. Please let me know who else to add here (@mfreed7?). It would be nice for us to add these to the other editor's drafts. I personally like the transparency of which editors are maintaining an editor's draft (good for pestering, too). :)

[melanierichards]

@mfreed7, not sure why you're not showing up in the list of potential reviewers, so please consider this a review request!

[una]

Hey! Left some thoughts in #357 RE: anchor positioning and our convo last week at the OpenUI meeting

[melanierichards]

Thanks @una, I'll take a look! Did you have any feedback on this first draft for popup? Hoping to land the first pass this week, so then we quickly can patch in updates as we discuss/resolve on popup-related issues.

[gregwhitworth]

Thanks for the PR looks great! In addition to the other questions/changes (primarily editorial) can you add a security section and outline the various mitigations you've added to the design (eg: in focus you reference origin restrictions, etc).

[gregwhitworth]

I feel like this could be re-worded to provide some contextual insights that this is due to the binding element being invoked not the actual popup. Maybe something like this:

or the popup element has been invoked through user interaction with the related element via the popup attribute

[melanierichards]

Made a change in 14defbf

[gregwhitworth]

Are we purposefully limiting this for the time being due to implementation/standardization complexity? EG: This would of course likewise work on date or color states.

[mfreed7]

I'm in favor of keeping this list small, at least to start. I'm not even sure about <input type=text>. You'd need to clarify exactly what you mean by "activated" there. Clicking within the control? Typing anything? Tab-focusing it? With buttons, I think it can be made pretty clear. For <input type=date> or <input type=color> that gets even more confusing (for users at least) since both of those typically pop up their own "popup" when activated. Do we want to show both popups in that case?

[melanierichards]

Yeah, I think we should limit to this current set of elements to start. The reasoning is not implementation complexity, it's more about which relationships make semantic sense (a commanding element vs a random div that is not perceivable to the user as interactive).

For date and color, I don't think we need the popup attribute because the controller code should take care of the interaction for the popup part.

[una]

+1 to a specified list of options for simplicity. What about number, search, and password types?

[melanierichards]

Thanks @una, I just blanked on a few! search makes sense, not sure what the use case would be for number but that seems ok. password we may want to omit for security purposes but I'm not totally sure on that one.

[gregwhitworth]

In Open UI we use the term component everywhere. While this is a meta thing I think we should revisit Open UI terminology to align with standards. Aria and CSS refer to them as Widgets, most authors refer to them as components so it would be good to have this meta discussion and resolve in some fashion. Possibly even get alignment between ARIA, WHATWG, CSS.

I'll file an issue

[mfreed7]

@gregwhitworth if you mean you want to use "component" instead of "elements" here, I'm not sure. These will all need to be proper HTML element tag names. Perhaps I'm misunderstanding your comment though.

[melanierichards]

Any action for me here? I'd say it's intentional that I'm using "interactive elements" in that way that HTML is using it, since I just want to reference that particular list of elements.

[gregwhitworth]

@melanierichards @mfreed7 only action is to align on definitions outlined here (and it's not blocking): https://open-ui.org/glossary

The implied definition of Interactive Elements is similar to our Control and that of ARIA's widget. So it's more that we as an industry should be referring to them as the same thing.

[gregwhitworth]

defer to the delegatesfocus spec definition

[melanierichards]

See https://github.com/openui/open-ui/pull/355/files#r652867167

[gregwhitworth]

nit: I prefer for if/else type algos to have expectation in sub-bullet and be indented in. This is similar to how the code will normally look and IMO is easier to read and follow. This may not be how WHATWG does it however (having the result be another bullet).

[mfreed7]

This is, unfortunately, how WHATWG typically writes if/else algorithms. Step N is "if X, do Y", and step N+1 is "otherwise, do Z".

[melanierichards]

+1, I elected to follow WHATWG style here. Inviting HTML editors to weigh in on whether they'd be amenable to sub-bullets or if we should follow house style in order to land this text with minimal changes.

[gregwhitworth]

What is this "returning" from?

[mfreed7]

returning from the algorithm.

Here, I'm not sure about WHATWG conventions. This is an if/elseif/else block, as step 4 is the "otherwise" for the embedded "if" in step 3. That, I haven't exactly seen.

[melanierichards]

Any changes requested, e.g. melding 4 into step 3?

[una]

Thanks @una, I'll take a look! Did you have any feedback on this first draft for popup? Hoping to land the first pass this week, so then we quickly can patch in updates as we discuss/resolve on popup-related issues.

Not a blocker! I still have a TODO for before the next OpenUI meeting tomorrow to review this draft in more detail

[mfreed7]

Very nice work! Thanks for putting this together! And sorry it took me so long to get around to doing the review.

[mfreed7]

returning from the algorithm.

Here, I'm not sure about WHATWG conventions. This is an if/elseif/else block, as step 4 is the "otherwise" for the embedded "if" in step 3. That, I haven't exactly seen.

[mfreed7]

Here is what I was referring to earlier. When light-dismissing, there will be a stack of open popups, and the light dismiss event doesn't have context to which popup should be subject. It will, however, have a target Node (in the case of a "click outside"). That Node should be the parameter used with the (new) hiding a popup element start node parameter. In code, for a "user presses ESCAPE" type light dismiss, I actually defined another algorithm to "hide the top popup" only. And for a "user scrolls the page" or "resizes the window" or other such light dismiss actions, you would call "hide the popup" with start node set to null, indicating that all open popups should be closed.

[melanierichards]

See above comment about solution: ecb4964

[mfreed7]

See comment above for this.

[melanierichards]

See above comment about solution: ecb4964

[melanierichards]

No worries @mfreed7, thanks for yours and Greg's review! Will address the comments as a batch and re-ping. :)

[melanierichards]

Thanks @mfreed7 @gregwhitworth @una for all the great feedback! This PR is now ready for another round of review. There are some open comment threads either because I had a question, wasn't sure about making a change, or would like review on the solution instead of resolving the comment myself. Please take a look and resolve/respond/let me know if you have any additional comments! Thanks again. :)

[gregwhitworth]

I requested an addition of a new issue but I think this is a solid landing for an editor's draft so we should land it.

Edit: Just noticed you already opened this - I added it to the agenda

[melanierichards]

Thanks @gregwhitworth for your review!

@mfreed7 @una I'm thinking it would be nice to merge this shortly and then continue making smaller incremental updates as needed. Sound good or is there anything you want to review/block on before we merge the first pass?

[domenic]

Sorry for the delay in reviewing this!

A few higher-level comments:

• This mdx format is unfortunately hard to read. I'd recommend using https://tabatkins.github.io/bikeshed/ if at all possible as it produces easy to read specs and has good auto-cross-linking capabilities. Although doing that would involve then another conversion to the language the HTML spec uses... maybe it'd be best to just work on a HTML spec PR directly?

• Be explicit about algorithm parameters using https://infra.spec.whatwg.org/#algorithm-declaration . Make sure all call sites pass all parameters to all algorithms.

• Be explicit about what the fields associated with a popup element are, and when they are changed or referenced. I think there are at least two: invoker and openness. They should each have <dfn>s, and then you can reference "popup's X" for a given popup.

• Avoid patching algorithms, like the focus algorithm or the showing a popup algorithm, from a distance. You seem to have organized things by feature, but algorithm steps need to be organized by algorithm.

I hope this helps!

[domenic]

It isn't clear what this means. Is this meant to take affect when the element is inserted into a document? Becomes connected? Becomes browsing-context connected? Is it synchronous or asynchronous?

That will also clarify nested popup cases; right now you are not providing any ordering (although the non-normative note below states one).

[melanierichards]

Added some changes to ccd9b75 to address this feedback (including higher up in the document). Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

Nit: modern style is "The show() method steps are to run the showing a popup element steps with this."

[melanierichards]

Added some changes to ccd9b75 to address this feedback. Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

There can be more than one element with that ID. You probably want to pick the first one in tree order. (I think tree order is correct, not shadow-including tree order?)

[melanierichards]

Good catch!

Added some changes to ccd9b75 to address this feedback. Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

It's best not to reference IDL attribute values since those can be messed with from JavaScript. Instead you should have some steps or an associated value you can reference. And then you can properly define the getter steps for the open IDL attribute.

[domenic]

Use loop syntax, i.e. https://infra.spec.whatwg.org/#iteration-while , to express iteration.

[melanierichards]

Added some changes to ccd9b75 to address this feedback. Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

invoker is not a declared variable... Maybe this is supposed to be a link to a definition?

[melanierichards]

Added some changes to ccd9b75 to address this feedback. Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

Suggested change

	given the _subject_ element to [fire a non-cancelable event](https://dom.spec.whatwg.org/#concept-event-fire)
	given the _subject_ element to [fire an event](https://dom.spec.whatwg.org/#concept-event-fire)

[melanierichards]

Added some changes to ccd9b75 to address this feedback. Please let me know if the changes resolve the issue or if you have any further comments; feel free to hold until I've resolved all open feedback and have formally requested a new review.

[domenic]

It's not clear what this means.

[melanierichards]

Can you help me understand what's unclear?

[domenic]

Well, first, do stacks grow up or down? :) It's not clear which is the top.

Second, which popup stack? This algorithm probably needs to take a document as an input, so you can look at that document's popup stack in particular.

[melanierichards]

Cool, added a reference to the popup's node document in ccd9b75, but I'll see what else I can do in the popup-stack section to clarify.

[domenic]

Do you want to ever remove it from the stack? Probably using pop? As written I believe it is left in the stack.

[melanierichards]

Step 6 calls out to an algorithm that would remove the subject from the stack (although I did need to make an update to make that clearer in ccd9b75). Note to self: update language from "remove" => "pop" to better fit house style.

[melanierichards]

Thanks @domenic for the review! I'll work my way through the comments and let you know if I have any follow-up questions.

Regarding the .mdx format: that's just what Gatsby/the Open UI CG uses. I think if we want to move to a different format, that's something the chairs should weigh in on. (cc @gregwhitworth @una) It's a bit tricky because the CG will incubate specs that will land in both HTML and CSS at a minimum, which of course differ in format. I'm not exactly sure what the best process solution is, but admittedly this PR is sort of a Frankenstein of formats. :)

[domenic]

Definitely getting up to spec!

[domenic]

I'm a bit unsure what this sentence is trying to say. I can imagine two interpretations:

• When a popup is browsing context-connected, do two things in sequence: first, set its open state to false; then, run the showing a popup element steps (presumably with only the popup as the first argument, and no second and third argument).

or:

• When a popup is browsing-context connected, set its open state state to false. Somewhere else in the spec will likely also cause the showing a popup element steps to run; if that happens, be sure to set the open state first.

Which is meant?

If its the former, it could be written a bit more clearly.

If it's the latter, that's not good; you should have a single set of browsing-context connected steps.

[melanierichards]

I suppose it's the latter, in which case a discussion on the correct resolution would be helpful. I'm thinking that this verbiage here can be dropped, and then a step 2 would be added to this algo: https://github.com/openui/open-ui/pull/355/files#diff-21cce170c953f236921ecbc4481a202782c1b2bc7464307c76aaa4dc60b64f11R131

The effect (paraphrasing in prose) would be: when the popup is browsing-context-connected, check for the initiallyopen content attribute. If present, set this open state to true and run through popup-showing steps. Else, set the open state to false. Would that change resolve the issue?

[melanierichards]

Ended up doing something like this in 5fc5041

[domenic]

What you did looks good!

[melanierichards]

Awesome, thanks!

[domenic]

I don't think it should call the show() method, since that might be overridden by user code (e.g. HTMLPopupElement.prototype.show = () => { throw new Error("boom"); }. I think instead you can just say something like "the popup element will be shown".

Notice also that you want to avoid "should". That's normative language that indicates a UA requirement, but I think this is just summarizing requirements that are stated more explicitly elsewhere in the spec.

[melanierichards]

Thanks, I'm not sure why I missed these shoulds. Perhaps because not written as SHOULD? :)

Anyway, addressed in 5fc5041

[domenic]

should or must?

[melanierichards]

Must! Addressed with 5fc5041

[domenic]

This step is not necessary; the algorithm is about to return anyway.

[melanierichards]

Addressed in 5fc5041

[domenic]

Probably you don't need to do a full tree walk to find out if subject is start node; just an equality check (in a separate step) would suffice.

[melanierichards]

Addressed in 5fc5041

[domenic]

Do you want to pass along invoker?

[melanierichards]

Actually yeah, even if it's just to set that to null. Addressed in 5fc5041

[domenic]

As before, setting IDL attributes directly isn't great.

[melanierichards]

Addressed in 5fc5041

[domenic]

I think this intends something like "set candidate subject's invoker to null"? (In which case no vague "If applicable" is needed.) Or is it intending to remove all DOM children of the invoker?

[melanierichards]

I think I'd copied this style from elsewhere in the spec, but the null intent is correct. Addressed in 5fc5041

[domenic]

Interesting. This allows autofocus to focus elements which are not normally focusable (as long as they are not inert). Is that intended behavior?

The normal autofocus behavior does not do this. It performs "getting the focusable area" on the element where autofocus is specified, which might return the element itself, or might return null. If it returns null, then the attribute does nothing.

[melanierichards]

Just pushed another batch of edits for HTML editor feedback in 5fc5041.

The following items are still open and in plan to address:

• https://github.com/openui/open-ui/pull/355/files#r687180355 (added a Q)
• https://github.com/openui/open-ui/pull/355/files#r675163363
• https://github.com/openui/open-ui/pull/355/files#r675188384
• https://github.com/openui/open-ui/pull/355/files#r675188930
• https://github.com/openui/open-ui/pull/355/files#r675189139
• https://github.com/openui/open-ui/pull/355/files#r687192423