Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[invokers] Should custom action names have naming requirements? #900

Closed
lukewarlow opened this issue Oct 20, 2023 · 15 comments
Closed

[invokers] Should custom action names have naming requirements? #900

lukewarlow opened this issue Oct 20, 2023 · 15 comments
Labels
invokers needs edits This is ready for edits to be made stale

Comments

@lukewarlow
Copy link
Collaborator

Should we require action names to be a dashed ident like "--action" or have a hyphen in them like "custom-action"?

I'm thinking of cases where someone might add their own action and then browsers want to add a new default action without breaking things?

@lukewarlow
Copy link
Collaborator Author

Just a thought this would also potentially serve as a (partial) feature detection (https://github.com/keithamus/invoker-buttons-proposal/issues/24).

If an invalid value that doesn't match the defined custom syntax is used for action then it would compute to auto?

@lukewarlow lukewarlow added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Oct 21, 2023
@lukewarlow
Copy link
Collaborator Author

The one compat issue this wouldn't necessarily solve is "auto" custom actions.

Currently there's the ability to do custom auto actions https://open-ui.org/components/invokers.explainer/#custom-behaviour

This issue wouldn't fix the issue where the platform wants to add an auto in future.

Is that an actual issue? Should we disallow firing of auto invoke events if no auto action is recognised on the target? That way they'd be forced to define an action?

Alternatively do we just encourage devs to always preventDefault() the events for custom use cases? That way there's no platform default happening if one is added in future?

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [invokers] Should custom action names have naming requirements?.

The full IRC log of that discussion <jarhar> Luke_W: i did have a question regarding custom actions, future compat question
<jarhar> Luke_W: invokers have a list of default actions and an idea of "auto"
<jarhar> Luke_W: if you put an action that doesnt correspond to anything you can still do stuff in js
<masonf> q+
<jarhar> Luke_W: you could end up in a situation where select doesnt have a showpicker function in html, i am working on it
<jarhar> Luke_W: someone could make a showpicker action which shows select through js
<jarhar> Luke_W: if we then came long later and added a showpicker action, the custom action and native one clashes
<jarhar> Luke_W: my thought process is should we require a dashed identity? maybe more of a css thing
<jarhar> Luke_W: should we require custom actions to have hyphens in it?
<jarhar> Luke_W: that forces you to match the spec
<jarhar> Luke_W: what are peoples thoughts on this? naming requirement doesnt fix auto case
<jarhar> Luke_W: if we dont have an action and we have something in auto, we still fire an event and it still becomes builtin
<jarhar> Luke_W: should we require preventdefault if you are trying to do something custom?
<keithamus> q+
<jarhar> Luke_W: people might not do it though and then we cant punish them afterwards for not doing best practices
<gregwhitworth> ack masonf
<jarhar> masonf: 100% agree there needs to be a namespacing difference, namespacing issue is real, we will want to add more new cool stuff in the future. if you dont do something to namespace it, it will be impossible later
<jarhar> masonf: second question of how. typical way would be requiring a dash like a custom element name, or a kind of data attribute has an attriubte in it at least
<jarhar> masonf: third question. usually none of those things are enforced. you can happily put a tag in your html that just works, and that causes problems. same for attribute values
<flackr> q+
<jarhar> masonf: i like the idea of breaking things when you dont namespace. goes counter to other things in html which are very permissive. i would love to make it stop doing that to make it obvious to developers that they didnt put a dash in
<gregwhitworth> ack keithamus
<jarhar> keithamus: issue i have with making some kind of namespace is that it makes polyfills more difficult if i cant dispatch an event to polyfil the behavior im stuck with some sort of weird solution
<jarhar> keithamus: kind of enforcing that users of polyfills have to do lots of migration, not just delete the polyfill and move on
<jarhar> keithamus: wonder if theres another solution to ... in the engine code you can say whether something is default preventable which is different to cancelable
<jarhar> keithamus: this has a default behavior which will be enacted. can we add a flag to events if this doesnt exit
<jarhar> keithamus: hopefully people who will polyfill can know if they need to prevent default
<gregwhitworth> ack flackr
<zcorpan> q+
<Luke_W> q+
<jarhar> flackr: was going to raise the polyfill point as well. i dont think theres much to add other than css is the only place where you cant polyfill css attributes because were not permissive with events or html or other things where you can closely polyfill them
<masonf> q+
<jarhar> flackr: then you can just remove the polyfill when you dont need it
<gregwhitworth> ack zcorpan
<jarhar> zcorpan: preventdefault is the thing you would use to cancel the event
<keithamus> q+
<jarhar> zcorpan: theres a return false alternative but its ... theyre both working with the canceled flag
<jarhar> Luke_W:
<gregwhitworth> ack Luke_W
<jarhar> Luke_W: one thought ive had: the correct way is that they preventdefault on the event and that will stop compat problems
<jarhar> Luke_W: the issue is forcing people to do that. if they dont do it it will break, and then we will have to break their website
<jarhar> Luke_W: can we have an event that is canceled by default?
<zcorpan> q+
<gregwhitworth> zakim, close the queue
<Zakim> ok, gregwhitworth, the speaker queue is closed
<jarhar> Luke_W: you could still run the default event but i cant think of a way to make it future compatible polyfillable and isnt reliant on devs doing the right thing
<jarhar> Luke_W: other than some way of saying this doesnt do anything by default
<jarhar> Luke_W: and calling it explicitly
<jarhar> Luke_W: the other thing is we dont do custom events at all to start with. not sure if that fixes the problem
<gregwhitworth> ack masonf
<jarhar> masonf: polyfills i do agree thats a thing. i dont think - im guessing the suggestion of inverting the event model is a giant change and confusing and hard to do and different from how the event model currently works
<jarhar> masonf: i like the fact that there is the custom - custom events do sound cool
<jarhar> masonf: we should make sure that we think a lot about the builtins right now so that v1 comes with the good ones
<jarhar> masonf: new ones in the future will be much harder to land. if someone else thinks of the cool one then we cant add it to the platform later
<gregwhitworth> ack keithamus
<jarhar> keithamus: the way i see the invokeaction is pretty much its just a - ergonomics around method dispatch
<jarhar> keithamus: some disparity like togglemodal doesnt exist but maybe we should add that
<jarhar> keithamus: same problem as methods. when we encounter that problem we just name the method different
<jarhar> keithamus: we could rename the action to something different if we encounter compat
<jarhar> keithamus: we should think about all of the existing behavior, what would the auto behavior be
<Luke_W> q+
<jarhar> keithamus: it should be like showing and hiding of elements
<jarhar> keithamus: there was issues raised around video: is it play and pause? is that the most default behavior?
<gregwhitworth> q?
<gregwhitworth> ack zcorpan
<jarhar> zcorpan: for event listeners, we dont want registering an event listener to have side effects or change behavior, thats an anti pattern
<jarhar> zcorpan: other thing is checkboxes and radio buttons have weird behavior when you cancel the click event
<jarhar> zcorpan: if you click the change state immediately, but if you cancel the click event they change state back
<jarhar> zcorpan: we dont want to copy this weird legacy behavior
<jarhar> zcorpan: only thing i can think of thats sort of like changed our mind after the fact
<jarhar> gregwhitworth: can we get a proposed resolution that captures this?
<jarhar> Luke_W: i think we need to take this back to the github thread
<jarhar> Luke_W: if we dont allow auto for custom actions...
<jarhar> masonf: can we resolve that we need a separate namespace? i feel like nobody argued aagainst that
<jarhar> Luke_W: that would break polyfilling
<jarhar> masonf: no, that there should be a separate defined namespace so that you used your own namespace
<jarhar> Luke_W: ah that firing an event is based on the namespaced
<jarhar> masonf: yeah if youre building your own thing you should use the namespace
<jarhar> zcorpan: so the namespace thing is only for events? i didnt follow
<jarhar> zcorpan: for events we also have different interfaces that you could use
<jarhar> gregwhitworth: maybe take proposed resolution back to the github thread
<jarhar> masonf: its a specified event object that has a paramter, values of that parameter, not the event name itself
<jarhar> masonf: i agree lets to back to the thread
<gregwhitworth> Zakim, end meeting

@lukewarlow
Copy link
Collaborator Author

Based on that discussion I'm thinking we should heavily encourage requiring a hyphen in custom actions.

I also think we shouldn't allow auto values for custom actions. One idea is that "auto" actually computes to the corresponding action name so the event would never show up with an action of auto? And if there's no defined action and no auto for the element maybe then we can afford to not trigger the event?

As for enforcement. If we didn't dispatch the event when they break these rules, we'd break polyfills. So the actual enforcement is a tricky one. But if we console warn and we document USE HYPHENS that might help us out.

While I personally think polyfills are great I also don't like the web having to come up with less good names for things just because developers have done the bad thing we told them not to. So I think there's a discussion to be had for strongly enforcing this.

@zcorpan
Copy link
Contributor

zcorpan commented Oct 26, 2023

How about a separate customAction IDL attribute intended for author-level actions, instead of naming convention in action?

@lukewarlow
Copy link
Collaborator Author

Where would that idl reside on the invoker element or the event? Could you explain a bit more how that would solve the compat issue please. Just so I'm sure I understand what you're proposing.

@zcorpan
Copy link
Contributor

zcorpan commented Oct 31, 2023

On the event, so is has both action and customAction.

@lukewarlow
Copy link
Collaborator Author

That's probably a good idea regardless of this issue. But I'm not sure if that helps with future compat concerns?

If we add a new action that conflicts with an existing custom one then wouldn't that still break things. Something that was a customAction would now be an action?

@gregwhitworth gregwhitworth removed the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Oct 31, 2023
@zcorpan
Copy link
Contributor

zcorpan commented Nov 1, 2023

Hmm yeah, it'd need to be invokecustomaction="" also I guess. I'm not sure it's better than naming convention.

@lukewarlow lukewarlow added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Nov 16, 2023
@lukewarlow
Copy link
Collaborator Author

Adding back to agenda to try and move forward discussion on this

@lukewarlow
Copy link
Collaborator Author

lukewarlow commented Nov 17, 2023

As for enforcement. If we didn't dispatch the event when they break these rules, we'd break polyfills. So the actual enforcement is a tricky one. But if we console warn and we document USE HYPHENS that might help us out.

I've just thought about this and actually it wouldn't break polyfills (full or future additional action).

If we only dispatch events for recognised defaults (potentially even scope it down to recognised defaults valid for the given target element) and valid custom actions (hyphenated), a polyfill would just have to fire the invoke event along with the relevant JS API itself? But they'd have to do that initially anyway?

Then we could provide console messages if an unrecognised default action is used.

We'd still compute actions to the value set (to allow polyfills to correctly read and use it). So the only puzzle piece left would be some sort of feature detection for supported actions per #907

@keithamus
Copy link
Collaborator

I'm supportive of this; dispatch events when we know we have a default for the target, or if there's a -. It's a great solution!

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [invokers] Should custom action names have naming requirements?, and agreed to the following:

  • RESOLVED: Require a custom action to have a hyphen, if the action is unrecognised and not custom, don't fire the event.
The full IRC log of that discussion <hdv> Luke: this is regarding if invoker's custom action names need to have naming requirements, like requiring a dash or other symbol
<hdv> Luke: there was some pushback last time with regards to polyfill
<hdv> Luke: have been thinking on it some more… proposal is if it's known by the browser that's good, if it's not known it's ignored… if it's not known by the browser but meets the requirement it could run… makes the polyfilling more difficult
<keithamus> q?
<keithamus> q+
<hdv> Luke: I think that's a good enough tradeoff
<gregwhitworth> +1, I don't think polyfilling should change the prior resolution
<masonf> q+
<gregwhitworth> ack keithamus
<hdv> keithamus: I think it's a great idea and solves the problem nicely
<hdv> keithamus: I don't think it makes the polyfill signifcantly more difficult, probably one line of code
<gregwhitworth> ack masonf
<hdv> masonf: would say +1 as well…  but why do you have to call out specifically the event is not recognised by the browser ? isn't it just has a dash vs doesn't have a dash
<hdv> Luke: depends on how we do it
<hdv> Luke: if it's not recognised by the browser and doesn't have a dash in it we don't fire it
<gregwhitworth> q?
<hdv> masonf: ok that confirms my understanding
<hdv> masonf: Simon brought an alternative to the issue. adding an IDL property to the event?
<hdv> Luke: I'm not sure if it solves the problem?
<hdv> masonf: would require another attribute?
<hdv> keithamus: the polyfill can't make the code look like it was native behaviour… if you were using a polyfill you would need to migrate your code
<Luke> Proposed Resolution: Require a custom action to have a symbol in the name, if the action is unrecognised and not a custom don't fire the event
<keithamus> +1
<hdv> s/the polyfill can't/issue with that solution would be the polyfill wouldn't be able to
<Luke> Proposed Resolution: Require a custom action to have a hyphen, if the action is unrecognised and not custom, don't fire the event.
<keithamus> +1
<masonf> +1
<brecht_dr> +1
<hdv> gregwhitworth: when you're parsing the HTML, you'll notice the hyphen and put it in some data store… should the proposed resolution be more explicit than custom?
<hdv> keithamus: implementation would probably be dispatch and if name is recognised, dispatch and use the name
<hdv> keithamus: would be no data store, just fall through logic
<hdv> masonf: you don't walk the tree, just check for an attribute
<Luke> RESOLVED: Require a custom action to have a hyphen, if the action is unrecognised and not custom, don't fire the event.

@lukewarlow lukewarlow added needs edits This is ready for edits to be made and removed agenda+ Use this label if you'd like the topic to be added to the meeting agenda labels Nov 30, 2023
Copy link

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.

@github-actions github-actions bot added the stale label May 29, 2024
@lukewarlow
Copy link
Collaborator Author

Going to go ahead and close this, as we got our resolution and the neccessry edits have been mde.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invokers needs edits This is ready for edits to be made stale
Projects
None yet
Development

No branches or pull requests

5 participants