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

EditContext: supported element list seems somewhat arbitrary #19

Closed
annevk opened this issue Dec 6, 2021 · 10 comments · Fixed by #44
Closed

EditContext: supported element list seems somewhat arbitrary #19

annevk opened this issue Dec 6, 2021 · 10 comments · Fixed by #44
Labels

Comments

@annevk
Copy link
Member

annevk commented Dec 6, 2021

Could you please explain what this list is based on? I think that ideally we reduce the number of "magic" HTML element lists.

cc @domenic

@travisleithead travisleithead transferred this issue from w3c/editing Apr 21, 2022
@dandclark
Copy link
Contributor

I asked the spec editors about this and it sounds like it was basically a placeholder, hand-picked from the flow content elements where any elements that were seen as tricky (e.g. <iframe>) were left out.

So we need to better formalize this. At a minimum we need to support <div> and <canvas> for the basic use cases of a DOM-based custom editor and a canvas-based custom editor.

I'm having a hard time finding an existing element list in the HTML spec that fits our needs. The list of attachShadow elements in the DOM spec could be a candidate:
"article", "aside", "blockquote", "body", "div", "footer", "h1", "h2", "h3", "h4", "h5", "h6", "header", "main", "nav", "p", "section", or "span"

We could name this list in the DOM spec, and then from the EditContext spec state that EditContext works with the "attachShadow list" + <canvas>. Not sure whether that's an improvement.

But thinking about this made me realize we have a further issue, where the spec doesn't say what happens when someone tries to assign to editContext on an "illegal" element. I guess it should always throw, but it's a bit odd to have a property on an element that can never be used for that element.

So I see 3 options to consider:
A) Assigning to the editContext property on a nonpermitted element throws.
B) Instead of putting the editContext property on HTMLElement, put it only on elements in the "allowed" list that we come up with.
C) Allow editContext to be assigned on every element, like how contenteditable can be assigned on any element.

I don't like option (C) because it could lead to some strange interactions: how should EditContext work for an <iframe>, a <input type="date">, or <select>? With contenteditable there seems to be some odd/buggy behavior with these on various browsers and I think we should sidestep all that if possible.

Option (B) looks better than (A) if the list of elements we want to support is on the smaller side.

Feedback on all the above would be very helpful, cc @alexkeng @snianu @chrishtr @annevk @masayuki-nakano

I'd also like to raise this on Thursday's meeting if there's time.

@annevk
Copy link
Member Author

annevk commented May 10, 2023

Why is the association API on the element instance and not the EditContext instance?

(A seems reasonable to me, especially if we follow attachShadow(), but also wondering why other API shapes were not considered.)

@bathos
Copy link

bathos commented May 10, 2023

B) Instead of putting the editContext property on HTMLElement, put it only on elements in the "allowed" list that we come up with.

There’s no interface more derived than HTMLElement for article, aside, footer, main, nav, and section elements, so it seems like that’s not an option if this is meant to be based on the (non-custom) shadow candidate host list.

@dandclark
Copy link
Contributor

Why is the association API on the element instance and not the EditContext instance?

The current design parallels contenteditable, but we're open to considering other options. I opened #40.

There’s no interface more derived than HTMLElement for article, aside, footer, main, nav, and section elements, so it seems like that’s not an option if this is meant to be based on the (non-custom) shadow candidate host list.

Yeah, for this option we'd need to either add property API to each element interface individually, or define a mixin that they could all pull in.

@bathos
Copy link

bathos commented May 11, 2023

Yeah, for this option we'd need to either add property API to each element interface individually, or define a mixin that they could all pull in.

The interface used for those I listed is HTMLElement itself. There is no HTMLMainElement interface, for example, to define new attributes/operations for, whether by mixin or not (i.e. there is no HTMLMainElement.prototype in the ES binding; a main element instance’s [[Prototype]] is HTMLElement.prototype itself). Do you mean define new interfaces for them? (Would that be considered backwards compatible at this point?*)

(Similarly, <blockquote> and <q> both use the HTMLQuoteElement interface, so one cannot add a member to only the subset of HTMLQuoteElement instances whose localName value is blockquote.)

* I don’t think it could be backwards compatible. Consider customElements.define("x-ample", class Example extends HTMLElement {}, { extends: "main" });. If HTMLMainElement became a thing, document.createElement("main", { is: "x-ample" }) would begin throwing a TypeError due to localName mismatch, as it would today if you extended span with HTMLElement as the super constructor instead of HTMLSpanElement.

@dandclark
Copy link
Contributor

Oops, you're right. I agree that option is probably off the table then. The API will have to be either on HTMLElement or EditContext.

@dandclark
Copy link
Contributor

In today's EditingWG meeting we resolved to adopt the "shadow root elements" list plus <canvas> as the supported elements list.

I will prepare a PR to the DOM spec to refactor the shadow root element list into something we can reference from the EditContext spec.

There is still an open question of what happens when someone tries to use an unsupported element.

[08:40] #19
[08:40] dandclark: anne raises a good point, we should formalize element list
[08:40] dandclark: use prior art from existing list?
[08:40] dandclark: bare minimum: support div and canvas
[08:41] dandclark: we do not want to be in the business of supporting EC on iframe, form controls
[08:41] dandclark: proposal: list of elements that work with attached shadow
[08:42] dandclark: "elements that are kind of boring"
[08:42] dandclark: just need canvas
[08:43] johanneswilm: list looks fine. on the first comment — would add span alongside div
[08:44] (the list is #19 (comment))
[08:44] chris: let's use the list of attached shadow elements + canvas
[08:44] johanneswilm: let's resolve on that

@bathos
Copy link

bathos commented May 11, 2023

@dandclark will custom elements be permitted as edit context “hosts” too, as with shadow roots?

@dandclark
Copy link
Contributor

That's my expectation, yes.

@dandclark
Copy link
Contributor

Pasting the notes from the 5/11/2023 Editing WG meeting, where we resolved to go with the attachShadow() elements plus <canvas>.

[08:39] issue 19, EditContext
[08:40] #19
[08:40] dandclark: anne raises a good point, we should formalize element list
[08:40] dandclark: use prior art from existing list?
[08:40] dandclark: bare minimum: support div and canvas
[08:41] dandclark: we do not want to be in the business of supporting EC on iframe, form controls
[08:41] dandclark: proposal: list of elements that work with attached shadow
[08:42] dandclark: "elements that are kind of boring"
[08:42] dandclark: just need canvas
[08:43] johanneswilm: list looks fine. on the first comment — would add span alongside div
[08:44] (the list is #19 (comment))
[08:44] chris: let's use the list of attached shadow elements + canvas
[08:44] johanneswilm: let's resolve on that

I'll submit a DOM spec PR to make the attachShadow() elements referenceable from the EditContext spec. Keeping this issue open to track that work.

I opened #40 to track @annevk's question about API shape.

dandclark added a commit that referenced this issue May 24, 2023
In #19 we resolved that the elements supported by EditContext should be those that work with attachShadow(), plus <canvas>.

Update the EditContext spec accordingly, and throw when the author attempts to associate an "unsupported" element with an EditContext.

Resolves #19.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 14, 2023
In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 14, 2023
In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
aarongable pushed a commit to chromium/chromium that referenced this issue Jun 16, 2023
In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617146
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Alex Keng <shihken@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1158908}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 16, 2023
In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617146
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Alex Keng <shihken@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1158908}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 16, 2023
In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617146
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Alex Keng <shihken@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1158908}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 22, 2023
…es, a=testonly

Automatic update from web-platform-tests
[EditContext] Limit to valid element types

In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617146
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Alex Keng <shihken@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1158908}

--

wpt-commits: 6388bf5fd752f0735e9bfc71196115c082e2b75e
wpt-pr: 40558
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Jun 22, 2023
…es, a=testonly

Automatic update from web-platform-tests
[EditContext] Limit to valid element types

In w3c/edit-context#19 the Editing WG
resolved that EditContext would support the same element types as
shadow root, plus `<canvas>`.

Update the implementation accordingly. See also the updated
spec at [2].

[1] w3c/edit-context#19
[2] https://w3c.github.io/edit-context/#extensions-to-the-htmlelement-interface

Bug: 999184
Change-Id: I2a8a040a279e14e9b98ece3370acdbdd8c3597ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617146
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Alex Keng <shihken@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1158908}

--

wpt-commits: 6388bf5fd752f0735e9bfc71196115c082e2b75e
wpt-pr: 40558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants