Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update to a spec draft document #251
Changes from 14 commits
4657165
df921b8
709f8de
9c805ec
6650990
31838e6
287485f
5577038
b607b9c
936bb59
c752195
2b5014b
aa88953
ba03b4e
20458af
c2ffdb8
8ee7cf4
6c46ff8
ac5ba6a
23bd69c
c1ddf21
cbd89f8
a53b2b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to push? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, let me check.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get the right license by default, but we need to enable "xref", for cross references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly to the issues above, I've changed it on my end.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReSpec will add this for free... you are welcome 😜
There was a problem hiding this comment.
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.
"its applicability" is unclear... what is "its" there?
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, you should move
<section id='conformance'>
to the end of the document. It's just boring boilerplate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren’t => aren't
There was a problem hiding this comment.
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:
You can then suggest changes inline that can be merged straight in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have this in the spec either... this could change in the future, and it's not appropriate to call out specific implementations in specification. Maybe move all this to a Wiki page here on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Whoa!!!! Noob question: is this web compatible??? I though "
-whatever
" was reserved for custom elements? Won't this conflict or cause confusion?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't this a losing proposition? A content author would need to forever keep updating this list of things to disable, while the UA is constantly trying to enable things.
Wouldn't an all or nothing approach be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoscaceres You mean a way to turn all off and then turn individual ones on again? We have gone back and forth between blacklisting and whitelisting on this. Browser makers generally prefer blacklisting and content authors generally prefer whitelisting. Browser makers are on the long end of the stick, so we went with blacklisting but with the addition that one can query which items are available, so that with a few lines of JavaScript one should effectively be able to implement whitelisting anyway.
This querying mechanism seems to have been removed with this PR and there is a tuicket here about bringing it back: #252
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made changes on my end and will push new changes soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML content attributes should first be approved by the WHATWG folks, and should be really be defined in HTML (the behavior of this stuff can be defined by this spec, however). @annevk and/or @domenic might want to give this a nod of approval.
We should then make a plan for how to integrate the specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute name violates https://w3ctag.github.io/design-principles/#casing-rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah, an issue against HTML with a rough outline of the plan/design would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoscaceres and @annevk thank you for your feedback. Issue tracking WHATWG discussion initiation.
I will remove the hyphen. Although,
contenteditabledisabled
seems lengthy to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: "unordered set of unique space-separated tokens" hans't been exported yet from HTML... still need to do that. In any case, please avoid liking directly to the HTML spec using
<a href=>
, always use[=whatever you want to link to=]
... and if[=whatever you want to link to=]
doesn't exist in the xref database, we need to raise an issue on the corresponding spec to get the term exported and added to the database.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ReSpec now throws the error. I will create an issue once the first change is landed so I can reference updated draft in the issue. Issue tracking the work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible, please alway avoid
<a href>
- worst case, usedata-cite=
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification is kinda sad that it gives zero assurances about anything 😿 It's SHOULD everywhere, which leads me to believe something is not right with the overall approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the background for this is that it has been mentioned by browser developers that some of the appearance is determined by the OS and that it's not always possible to fully remove certain menus and menu entries. So in those cases they will grey those out in instead or make them visually disabled in some other way.
But using "SHOULD" everywhere also basically means that a browser can be fully compliant with the spec without actually having implemented anything at all, right? In that would it make more sense to write that it "MUST" disable menus in some way without specifying what that way is and those browsers that cannot comply with that then need to be listed as non-compliant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's interesting. You are saying the functionality gets disabled... so, that could work: It's not so much the presentation/UI aspect that the spec needs to concern itself with, but with the functionality itself... then it can be "MUST" just around the functionality.
Yeah.
So yeah, don't think about this as menus (because then it crosses into areas where browsers compete at the UI level)... just talk about functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could say that user agents MUST NOT display UI that indicates an editing command is applicable if that editing command's corresponding Input Type is present in the value of the contenteditabledisabled attribute?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, please see also:
https://github.com/w3c/respec/wiki/data--number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I created an issue and tagged with Agenda+ to be discussed tomorrow.
There was a problem hiding this comment.
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 :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.