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

Do we really want forcing adding prefixes to inputType? #39

Open
chong-z opened this issue Sep 29, 2016 · 4 comments
Open

Do we really want forcing adding prefixes to inputType? #39

chong-z opened this issue Sep 29, 2016 · 4 comments

Comments

@chong-z
Copy link
Contributor

chong-z commented Sep 29, 2016

Issue Background

Recent commit 28b3edf adds prefixes to all inputType, e.g.

'transpose' => 'insertTranspose'

'undo' => 'historyUndo'
'redo' => 'historyRedo'

'bold' => 'formatBold'
'subscript' => 'formatSubscript'
'justifyCenter' => 'formatJustifyCenter'
'removeFormat' => 'formatRemove'

The original point of adding prefixes came from TPAC 2016: https://www.w3.org/2016/09/22-webapps-minutes.html#item04 @garykac

Gary: If we strictly use insert and delete as prefixes for events, maybe format* as well, makes it much easier to catch all events.


Original Naming style

Actually those prefixes look weird to me, and I'm not sure how useful "catch all events" would be.

The original style is DOMAction+Source/MoreDetails, and By/From means 'indirect outcome'. e.g.

* `deleteWordForward` means user initiated a 'delete' action, which is meaningful enough by itself. 
JS can either
  1. Filter prefix 'delete' and delete target ranges, or
  2. Filter 'Word' and implementing it's own algorithm to calculate target ranges
See how [CKEditor Typing Demo](https://github.com/ckeditor/ckeditor5-typing/blob/45b47a3cc68e36932592ebcc151b12ea79f149d5/src/delete.js#L53) utilize target ranges for selected units.

* `deleteByCut` means user initiated a 'cut' action, and this `beforeinput` is only about the 'delete' part of the action.

* `bold` just mean action `bold`, and there is no additional information.
(We probably don't want `boldFromKeyboard` or `boldFromMenu`)
  • We have lots of insert* and delete* because they can be handled using similar logic (if JS doesn't care about sources, etc).
    • e.g. If we decided to add new type insertFromSpeech later, it should just work with existing insert logic
  • We have other standalone inputType because they have to be handled individually
    • e.g. Adding prefix format or history doesn't provide useful information, JS still need to make a list of supported inputType, and preventDefault()/Let go on others

Proposal

We could do something about transpose, undo and redo, but should leave other inputType standalone.
(As well as backColor, createLink, fontName, and fontColor proposed here #32 (comment))

  • For transpose: insertTranspose or insertFromTranspose could work, but I'm not sure about semantics...
  • For undo and redo: We need to make it clear it's only about DOM modification, e.g. undoModification, modificationFromUndo, see replace undo/redo with events that only change DOM? #21
@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 29, 2016

As you mention, this issue was up at the F2F. The proposal, which as you state came from @garykac, was to either prefix all those remaining inputtypes that didn't have a prefix or to create two levels: supertypes and subtypes. We didn't have a clear conclusion for which one of the two we would pick, but noone seemed opposed to the idea that we should do one of the two. I added the prefixes now, but was prepared to switch it all to a super/sub type model if that ended up being the final conclusion.

I personally don't care for which one of the three options that are now on the table we pick. I don't think "looks weird" is a good argument, but I also agree that there are likely not many "shortcuts" JS developers can take by checking for prefixes or supertypes. They'll spend quite a bit of time developing on top of this anyway, so they will have the time to distinguish the inputtypes for most purposes. An exception may be insert paragraph and insert linebreak. Some editors may choose to treat these two the same way so having a prefix or supertype may help them.

Prefixing could also have the purpose to group them more easily together. Either to understand the spec or by having different subfunctions for inserting, deleting, history or formatting. But again - the time saved by this is minor.

Given that the strongest opinions on this matter both seem to have their origin in Google, and not that many others have expressed much of an opinion on this matter, maybe you guys could come up with a common proposal and then present to the rest of us?

@chong-z
Copy link
Contributor Author

chong-z commented Sep 29, 2016

I don't think "looks weird" is a good argument

Sorry for the confusion, the argument should be:

  • Adding prefix format and history doesn't provide additional useful information, so we shouldn't do it.

But yes it won't mean a lot for JS, so If we are prefixing

  • for the sake of spec, OK
  • otherwise I think we shouldn't do it because we think JS could benefit from it

@garykac @ojanvafai WDYT?

@garykac
Copy link
Member

garykac commented Sep 29, 2016

We have a bunch of commands that fall into obvious categories, and we kinda-sorta use prefixes for some of them (deleteXxx and insertXxx).

This implies that there is some structure there (the categories). If the commands do fall into categories, then it would be silly to ignore that structure and dump them all into an unorganized bag of commands.

A developer that wants to write code to filter out all formatting commands shouldn't have to enumerate each and every formatting command (which will break if we add more commands).

if prefix(type) == format

is a lot better than

if type == bold or type == italic or type == underline or type == strikethrough ...

And having a list with common prefixes also makes the list easier to read since related items are all grouped together rather than scattered throughout the list of commands.

The argument that it doesn't provide additional useful information is not valid. It does provide additional information: the category. While we may happen to know that "bold" is implicitly a "format"ing command, this fact is not obvious to the code unless we make it explicit, and exposing the category to the code is the point of having the prefix.

Without these explicit categories, many devs are going to end up creating tables for these categories:

formattingCommands = ['bold', 'italic', ...];

and it's very easy to imagine people wanting to write code like the following:

if prefix(type) == 'history'
   undoManager.command(type, data);
else if prefix(type) == 'format'
   // Ignore all format commands
else if ...
...
else
   log('unknown command: ' + type);

The key is that it's easy for us to choose sensible names now. It'll be harder to fix the names later. We can address this categorization-problem either by (1) ignoring it, (2) having type/subtype attributes (probably overkill), or (3) using a common prefix for the categories. The latter option is much easier to do.

@annevk
Copy link
Member

annevk commented Sep 30, 2016

It seems better to expose two separate enums in that case: category and command. You might have different kind of groupings going forward after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants