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

Extending the list of supported elements #103

Open
Comandeer opened this issue Jul 12, 2024 · 8 comments
Open

Extending the list of supported elements #103

Comandeer opened this issue Jul 12, 2024 · 8 comments

Comments

@Comandeer
Copy link
Member

We started to experiment with EditContext API in CKEditor 5 and encountered an issue. There are so-called "nested editables" inside the editor – smaller editable regions inside the main editable region of the editor, e.g. image captions or table cells. We want to use the EditContext API also in these nested editables but it can't be used directly due to limiting the list of supported elements only to shadow hosts + <canvas>. This limitation forces us to dynamically inject a <div> element into a nested editable to enable the EditContext API when the user starts interacting with it:

<figure class="image" contenteditable="false">
	<img src="[…]" alt="[…]">
	<figcaption contenteditable="true">Some caption</figcaption>
</figure>
<!-- beomes -->
<figure class="image" contenteditable="false">
	<img src="[…]" alt="[…]">
	<figcaption contenteditable="true">
		<div class="edit-context-host">Some caption</div>
	</figcaption>
</figure>

It works but introduces some drawbacks, especially when it comes to the styling, e.g. it breaks selectors like td > p:first-child.

It was decided in #19 to limit the list to "boring elements" (shadow hosts) but I feel that this list could be too limited. CKEditor 5 is not the only editor that uses nested editables, e.g. Tiny also uses them with the <figcaption> element for image captions so does Lexical. Due to that, I propose extending the current list of supported elements, e.g. to all elements supporting the contenteditable attribute.

@Comandeer Comandeer added the Agenda+ Queue this item for discussion at the next WG meeting label Jul 12, 2024
@dandclark
Copy link
Contributor

contenteditable is a global attribute so it can technically be applied to everything, even elements like <iframe> or <input> where it doesn't really make sense. So I don't think we can use that a guide. But I agree that elements like <td> or <figcaption> seem reasonable to support.

Brainstorming what a new deciding principal could be, maybe we could support every element except for those that can be any of:

@Comandeer
Copy link
Member Author

LGTM but it still allows some "weird" elements as phrasing content includes e.g. progress. Probably we would need to add some additional restrictions, e.g. exclude labelable elements.

@dandclark
Copy link
Contributor

These are the minutes from the August 8 2024 meeting on this topic:

08:32 johanneswilm: next issue: #103
08:32 johanneswilm: this is dandclark and comandeer
08:32 comandeer: we've started to experiment with EC in CKE. we have nested editable roots
08:33 comandeer: we want to use EC there, with nesting..
08:34 dandclark: we picked a non-arbitrary list based on Shadow DOM UA, since there's precedent
08:34 dandclark: however, figcaption seems reasonable to deploy EC inside of
08:34 dandclark: can we go the other way — start with all elements and subtract away a subset?
08:35 dandclark: no script elements, some form controls, object/iframe for example
08:35 dandclark: next thing — do exercise of coming up with comprehensive list
08:35 dandclark: easy to expand, not easy to claw back. we should be sure
08:36 johanneswilm: w.r.t. this nested editing behavior... there's two ways of doing this with EC
08:36 johanneswilm: either multiple nested EC, or have a single EC but preventDefault() (or equivalent thereof) on the fly
08:37 comandeer: we currently use multiple EC, multiple levels
08:38 johanneswilm: (are these the only two options? should we recommend one or the other?)
08:38 johanneswilm: would it be worth having an explanatory note in the spec, to clarify this?
08:38 dandclark: having separate EC instances seems like the natural way to go about this
08:39 dandclark: there's separate editable fields and they logically have different text content
08:39 dandclark: as long as they're actual separate fields where you can't delete and delete text in another field, they should probably be separate contexts too
08:39 dandclark: ...but both are reasonable, ultimately
08:40 johanneswilm: dandclark will come up with comprehensive list next mtg

@dandclark
Copy link
Contributor

Here is the potential list that would be produced based on the result of the prior discussion in this issue:

abbr, address, area, article, aside, b, bdi, bdo, blockquote, body, br, canvas, caption, cite, code, col, colgroup, data, datalist, dd, del, dfn, dialog, div, dl, dt, em, fieldset, figcaption, figure, footer, form, h1, h2, h3, h4, h5, h6, head, header, hr, html, i, ins, kbd, legend, li, main, map, mark, menu, nav, ol, optgroup, option, p, pre, q, rp, rt, ruby, s, samp, section, slot, small, source, span, strong, sub, sup, summary, table, tbody, td, tfoot, th, thead, time, tr, track, u, ul, var, wbr

I generated this by:

This can be reproduced with: https://jsfiddle.net/dandclark_msft/d846Lk3p/55/

This still leaves some I'm hesitant to add support for but that don't fall into any category that we can eliminate without also eliminating others that we need to include. The potentially weird ones I see are: 'area', 'dialog', 'html', 'form', 'map', 'optgroup', 'option', 'slot'.

We may just have to list these as special cases.

@smaug----
Copy link

That list should still contain valid custom element names.

Not sure about col and colgroup, there might be some other weird ones.

@dandclark
Copy link
Contributor

The minutes from today's call:

08:03 issue: https://github.com/w3c/edit-context/issues/103
08:04 smaug: why did we limit element types in the first place?
08:04 sanket: support canvas, otherwise just match contenteditable
08:05 *** sanketj_ (~[email protected]) has joined the channel
08:06 dandclark: proposed element list includes things like area, map, dialog, html
08:06 (https://github.com/w3c/edit-context/issues/103#issuecomment-2344923844)
08:06 *** johanneswilm (~[email protected]) has joined the channel
08:06 dandclark: you can set cE on any of these elements today, and browsers try to do reasonable things. behavior is inconsistent though (e.g. setting cE on iframe element)
08:08 dandclark: two paths — use original list in comment #103, or do more conservative approach of limiting EC to smaller subset
08:09 johanneswilm: if we keep them supported on an element, do we expect it to be effectively usable? (e.g. EC on slot or HTML)
08:09 dandclark: probably do something reasonable — slot for instance may not become editable, HTML might result in same behavior as cE on HTML
08:10 smaug: we should consider custom elements
08:10 dandclark: we already allow those
08:10 smaug: what about cE/EC to table?
08:12 johanneswilm: there's a risk where if we enable it on certain "stranger" elements, people might find esoteric use cases. e.g. being able to modify a visible display: block; style element to drive live style updates in the page
08:13 *** Di (~[email protected]) has joined the channel
08:13 johanneswilm / dandclark: proposal is to go with the list in https://github.com/w3c/edit-context/issues/103#issuecomment-2344923844, and remove some subset of elements that just don't make sense. consider adding head/style to that list
08:14 dandclark: let's find others that should be excluded, and err on the side of caution

@dandclark
Copy link
Contributor

dandclark commented Sep 26, 2024

The list we will have for now:
See above comment for initial list, with these modifications resolved in TPAC meeting:
Ruby: Yes
Grouping: No, except for figure
Table: No, except for caption, td, th
Details, summary: No
All no: area br datalist dialog form head hr html map optgroup option slot source track wbr
(custom elements are also yes, except for customized builtins; same as attachShadow rules)

@johanneswilm
Copy link

From TPAC 2024 minutes:

Dan: which elements do we want to allow for editContext. We made previous selection - and that was not enough (

didn’t allow it). Question is which ones. We started out with all HTML elements, and then removed specific groups where it wouldn’t make sense. I think we need to remove a few individual ones. We can't rely on HTML categories to select these tags. What does group think?
I removed a number of different ones. I think Ruby should have them. If it’s not rendered, then it just won’t receive.

Anne: why do we have a restriction at all?

Dan: To avoid weirdness with strange elements. MIght also be compatibility issues. Thinking of a safe list. Try to start from list of all HTML elements, eliminating those where it doesn’t make sense.

Ryosuke: May be an idea to refer to a list of elements with shadow dom.

Dan: These are included, but then we have some extra. Unfortunately, I think there will be a list of exceptions, unfortunately.

Anne: … generally we should rely on these list of tags for implementation requirements. I think we should make a list of elements and then write that this matches these categories.

SImon: Excluding elements may exclude usecases we haven’t thought of. Such as contenteditable on style element.

Johannes: We had actually thought of that. And were a bit afraid of the consequences.

Anne: Ideally, we figure out what the actual thing is, then we can just allow everything like contenteditable. We need to figure out what the behavior for contenteditable anyway.

Simon: Would be good to be in line with contenteditable. As we will need to figure the behavior out for contenteditable anyway.

Dan: summary is out. Every table-related element is out. Is it ok to exclude these?

Anne: Sounds reasonable but I will defer to Ryosuke and Megan

Ryosuke: I think td and th should be allowed. The editontext does not directly insert dom elements. So it should be possible to do something reasonable.

Anne: If contenteditable on tables isn’t interoperable, then probably a good idea to exclude here. Do we really need a list?

Dan: eventually interoperable. Until then it’s good to hold back.

Ryosuke: …

Anne: If it primarily depends on layout and not element type, we do we have a list?

Ryosuke: How about flex/grid?

Dan: We should test it. I think it’s difficult to exclude based on layout. What do we do if the layout of an element is changed? Do we kick the editcontext off?

Olli: What do we actually test?

Dan: caret positions, getTargetRanges()

Ryosuke: We check the positions and see if they match.

Anne: I don’t mind a list if we write the tests?

Johannes: we are not sure editcontext whether the DOM changes or the changes on the page that happen in the element that is associated with the editcontext to be. You could have some random element
that is associated with editcontext and then render everything in totally different elements. So list of allowed ones might not effectively stop users from using context on other elements.

Dan: Next category figure/fieldset/menu/ol/ul . Are they interoperable?

Sanket: Ol/Ul seem similar to table, probably should leave them out. Not sure how to think about figure.

Johannes: for figure, you put contenteditable false then on the fig caption you put the contenteditable true. To avoid the figure is a mathematical formula instead of an image

Simon: what’s the problem with figure?

Johannes: typically to avoid people to write outside of the caption

Simon: we just need to revisit the list over and over because people are trying to do tings that we didn’t think of

Anne: not point spending a lot of time on this.

Johannes: No point in having browser devs spend a lot of time developing things that are not generally used.

Dan: Ok, so conclusion: yes, to figure. No to the remaining ones.

Anne: Test needs to see what bounding boxes are like in duiffernet layout types.

Ryosuke: + selection extension. Really good to have a test for selection API as well.

RESOLUTION: we go for safe list (see github comment)

@johanneswilm johanneswilm removed the Agenda+ Queue this item for discussion at the next WG meeting label Oct 7, 2024
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