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

Update to a spec draft document #251

Closed
wants to merge 23 commits into from
Closed

Conversation

gked
Copy link

@gked gked commented May 7, 2020

Closes #176

The following tasks have been completed:

This PR addresses current vision of the shape of contenteditable-disabled proposal.
In previous discussions, the group talked about

  1. disabling UI that is not available in the editor (e.g. markdown scenarios for font commands)
  2. controlling Undo/Redo UI
  3. disabling some input UI that can show up on a page unexpectedly on user focus. e.g. voice typing UI that Windows team is experimenting with.

We've realized that a more efficient solution for 2 would be to take advantage of Undo API to to communicate to UA correct state of UI at all times.

Edge team has internally discussed 3rd use case and has a number of open questions around if it's the right fit for contenteditable-disabled. Resolution was to spend more time thinking about it.

As a result current draft is only about disabling UI (formatting UI only).

@gked gked requested review from johanneswilm and marcoscaceres May 7, 2020 22:13
<section id="introduction">
<h2>Introduction</h2>
<p>
In contenteditable regions of documents, some user agents provide contextual user interface (UI) elements to help accelerate common editing operations. The UI is meant to enhance the editing experience, but when authors create customized editors that aren’t compatible with user agent UI, it leads to confused users.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren’t => aren't

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johanneswilm don't forget you can use GitHub's funky new suggestion button:

Screenshot 2020-05-08 14 56 31

You can then suggest changes inline that can be merged straight in.

}
],
github: "https://github.com/w3c/editing",
shortName: "contenteditable-interface",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed the short name doesn't match. Should this be contenteditable-disabled too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd think so.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to push? 👀

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, let me check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been pushing my changes through my coworker's fork and the changes are not flowing in, somehow, I am making an attempt to edit the file in the PR.

Copy link
Contributor

@johanneswilm johanneswilm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @gked and @BoCupp-Microsoft ! I noticed you removed supported. This was something we had added after discussing whether to blacklist or to whitelist to kind of allow whitelisting anyway while giving a preference for blacklisting.

The reason this is important to JS editors is that it took several of the JS editor companies several months to discover that the Mac touch bar had been launched. During this time, their users would use the buttons of this bar and nothing would happen in the users editors or even worse - the editors were breaking. The same can happen again with new devices launches with new versions of browsers and operating systems that not every development team is aware of immediately. For that reason there was a demand for a way to query all the supported options that then can be disabled subsequently.

Maybe we need to open a ticket about this?

</p>
<p class="issue" title="missing fontSize and insertImage">
fontSize / insertImage is missing from beforeInput. Why? It seems that it should be listed to create a more complete set.
<a href="https://github.com/w3c/editing/issues/249">Issue 249</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe if you use data-number ReSpec will link the issue for you. I find these features by clicking the ReSpec button, then clicking about ReSpec, and then clicking the link that sounds most like what I want to do. In this case it took me here: https://github.com/w3c/respec/blob/develop/src/core/issues-notes.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<pre class="idl">
[Exposed=Window]
interface ElementContentEditable {
[SameObject, PutForwards=value] readonly attribute DOMTokenList contentEditableDisabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This readonly might be incorrect if we have a PutForwards. Need to check the webidl spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually that looks correct: https://heycam.github.io/webidl/#PutForwards

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JPG version of ios-editing-buttons looks unused and can be deleted.

fixed.

};
</pre>
<p>
The <code>contentEditableDisabled</code> WebIDL property reflects the contenteditable-disabled DOM content attribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some things in this spec that should be definitions and others that should be linking to those definitions. Need to determine how to make that happen in ReSpec. For example, contenteditable-disabled DOM content attribute should probably be linking back to the definition for that phrase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I've created an issue to fix DOMTokenList linking, I can use it for all dfns related fixes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @gked and @BoCupp-Microsoft ! I noticed you removed supported. This was something we had added after discussing whether to blacklist or to whitelist to kind of allow whitelisting anyway while giving a preference for blacklisting.

The reason this is important to JS editors is that it took several of the JS editor companies several months to discover that the Mac touch bar had been launched. During this time, their users would use the buttons of this bar and nothing would happen in the users editors or even worse - the editors were breaking. The same can happen again with new devices launches with new versions of browsers and operating systems that not every development team is aware of immediately. For that reason there was a demand for a way to query all the supported options that then can be disabled subsequently.

Maybe we need to open a ticket about this?

Sounds good, I created an issue and tagged with Agenda+ to be discussed tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When xref option is enabled, DOMTokenList will link correctly :)

@BoCupp-Microsoft
Copy link
Contributor

The JPG version of ios-editing-buttons looks unused and can be deleted.

<section id="introduction">
<h2>Introduction</h2>
<p>
In contenteditable regions of documents, some user agents provide contextual user interface (UI) elements to help accelerate common editing operations. The UI is meant to enhance the editing experience, but when authors create customized editors that aren’t compatible with user agent UI, it leads to confused users.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In contenteditable regions of documents, some user agents provide contextual user interface (UI) elements to help accelerate common editing operations. The UI is meant to enhance the editing experience, but when authors create customized editors that arent compatible with user agent UI, it leads to confused users.
In contenteditable regions of documents, some user agents provide contextual user interface (UI) elements to help accelerate common editing operations. The UI is meant to enhance the editing experience, but when authors create customized editors that aren't compatible with user agent UI, it leads to confused users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another commit. It caused github to disable Commit Suggestion button. I'll make a change on my and push it here.

license: "w3c-software-doc"
};
</script>
<script src='https://www.w3.org/Tools/respec/respec-w3c-common' class='remove'></script>
Copy link
Member

@marcoscaceres marcoscaceres May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: common profile is deprecated...

Suggested change
<script src='https://www.w3.org/Tools/respec/respec-w3c-common' class='remove'></script>
<script src='https://www.w3.org/Tools/respec/respec-w3c' class='remove'></script>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another commit. It caused github to disable Commit Suggestion button. I'll make a change on my and push it here.

<section id='abstract'>
<h2>Abstract</h2>
<p>
This specification defines an API that indicates to user agents which editing-related commands are applicable to contenteditable regions of a document. User agents SHOULD take this information into account when presenting any editing-related UI to ensure its applicability.
Copy link
Member

@marcoscaceres marcoscaceres May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid RFC2119 keywords in non-normative sections.

Suggested change
This specification defines an API that indicates to user agents which editing-related commands are applicable to contenteditable regions of a document. User agents SHOULD take this information into account when presenting any editing-related UI to ensure its applicability.
This specification defines an API that indicate which editing-related commands are applicable to `contenteditable` regions of a document. User agents can take this information into account when presenting any editing-related UI to ensure its applicability.

"its applicability" is unclear... what is "its" there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'its' is referring to editing-related UI in that sentence. The API provides hints as to what editing operations (and therefore what editing UI) are applicable in a given contenteditable element. We could change it to... "User agents can take this information into account when presenting any editing-related UI to ensure it is applicable to a given contenteditable region."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another commit. It caused github to disable Commit Suggestion button. I'll make a change on my and push it here.

This specification defines an API that indicates to user agents which editing-related commands are applicable to contenteditable regions of a document. User agents SHOULD take this information into account when presenting any editing-related UI to ensure its applicability.
</p>
<p>
This specification purposefully does not prescribe the means by which a user agent will indicate to the user that some commands are not applicable in a given contenteditable region.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence reads like a double negative.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe... "This specification purposefully does not prescribe the means by which a user agent will indicate to the user the applicability of any editing command. The user agent could, for example, omit editing UI, disable it, or choose some other appropriate UI treatment."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am taking Bo's suggestion and will push the changes in the next iteration.

Comment on lines 153 to 156
<p class="issue" title="missing fontSize and insertImage">
fontSize / insertImage is missing from beforeInput. Why? It seems that it should be listed to create a more complete set.
<a href="https://github.com/w3c/editing/issues/249">Issue 249</a>
</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p class="issue" title="missing fontSize and insertImage">
fontSize / insertImage is missing from beforeInput. Why? It seems that it should be listed to create a more complete set.
<a href="https://github.com/w3c/editing/issues/249">Issue 249</a>
</p>
<div class="issue" data-number="249">
</div>

};
</pre>
<p>
The <code>contentEditableDisabled</code> WebIDL property reflects the contenteditable-disabled DOM content attribute.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The <code>contentEditableDisabled</code> WebIDL property reflects the contenteditable-disabled DOM content attribute.
The {{ElementContentEditable/contentEditableDisabled}} IDL attribute reflects the contenteditable-disabled DOM content attribute.

@marcoscaceres
Copy link
Member

Just a note for the editors... the following resource might help you with ReSpec usage:

https://github.com/w3c/respec/wiki/Shorthands-Guide

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things we should sort out... let me know if we should split them into followup bugs or not.

@saschanaz, you wanna go through this also?

@saschanaz
Copy link
Member

This is super hard to read diffs, could we restore the name and rename in a separate PR?

@marcoscaceres
Copy link
Member

@saschanaz, might be ok to treat this as an entirely new spec and ignore the diff? The spec itself is pretty small and think we’ve identified some showstoppers already.

@saschanaz
Copy link
Member

@saschanaz, might be ok to treat this as an entirely new spec and ignore the diff? The spec itself is pretty small and think we’ve identified some showstoppers already.

I think showstoppers don't force abandoning diffs, though.

@marcoscaceres
Copy link
Member

I guess. Whatever is easiest.

@marcoscaceres
Copy link
Member

Stupid question... but y u no contenteditable=“disabled”? Is that because any truthy attribute value enables it? Even if so, maybe one can feature detect for it... like set it to “disabled”, if it becomes “”, then remove the attribute. Otherwise, it’s supported. Just a thought.

Active Documents/contentEditableDisabled.html Outdated Show resolved Hide resolved
</p>
<p class="issue" title="missing fontSize and insertImage">
fontSize / insertImage is missing from beforeInput. Why? It seems that it should be listed to create a more complete set.
<a href="https://github.com/w3c/editing/issues/249">Issue 249</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should reflect the discussion result. Maybe just a note that those are missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this at the meeting and agreed to remove them from both specs for now.

</ul>
</p>
<p class="note">
Note that this specification does not have a shortcut to disable all editing UI. Authors should consider using a <code>textarea</code> or <code>contenteditable="plaintext-only"</code> for this purpose.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think plaintext-only shouldn't be recommended as it's currently removed from the spec.

Active Documents/contentEditableDisabled.html Outdated Show resolved Hide resolved
Active Documents/contentEditableDisabled.html Outdated Show resolved Hide resolved
Grisha Lyukshin and others added 3 commits August 11, 2020 07:07
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
@gked
Copy link
Author

gked commented Aug 11, 2020

@marcoscaceres and @saschanaz: apologies for leaving this PR hanging. I just got back from a 2 month leave. This PR became really messy in terms of the changes. If you don't mind, I'll abandon it and re-create a new PR with all of the comments here addressed.

@saschanaz
Copy link
Member

saschanaz commented Aug 11, 2020

If you don't mind, I'll abandon it and re-create a new PR with all of the comments here addressed.

Sounds good 👍

@johanneswilm
Copy link
Contributor

@gked @BoCupp-Microsoft What is the status here? Can we get these changes merged? The document has been moved since so we'll need to update the PR - or have it all be merged through an alternative PR as you mentioned earlier.

@marcoscaceres marcoscaceres requested review from marcoscaceres and removed request for marcoscaceres July 26, 2021 04:41
@johanneswilm
Copy link
Contributor

@BoCupp-Microsoft @travisleithead I will put this on the agenda for the next call. If it can be resolved before then, we can remove it from the agenda again. Basically, the question is whether there is a new PR being developed somewhere that addresses the feedback that was collected in this PR or whether that is not the case. If it is not the case, should we put it on someone's todo list to create such a PR?

@johanneswilm johanneswilm added the Agenda+ Agenda item to be inserted in the Editing TF meeting queue label Aug 5, 2021
@travisleithead
Copy link
Member

travisleithead commented Aug 6, 2021

@johanneswilm yes, it looks like PR #265 is actually the "non-messy" version of this PR that @gked promised. I think it's safe to close this PR unmerged at this point and focus on the one. (I'll transfer your Agenda+ label to that PR so that it can be discussed next week too.

@travisleithead travisleithead removed the Agenda+ Agenda item to be inserted in the Editing TF meeting queue label Aug 6, 2021
@travisleithead
Copy link
Member

Closing this PR in favor of #265

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

Successfully merging this pull request may close these issues.

contenteditable-disabled
7 participants