-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support for ElementInternals
and Constraint Validations
#1190
Conversation
ebdd64f
to
33165de
Compare
@jorgemanrubia this is a follow up to #1188. Are you able to review? |
33165de
to
30cb95e
Compare
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.
Lovely @seanpdoyle, thanks.
I'd like to cut a version tomorrow to include #1191, since that's a bad one (we're currently QAing the change). I'll merge this one next week, since I don't want to include more changes for tomorrow.
5e9cecc
to
edfccf6
Compare
ElementInternals
and Constraint Validations
a3cbff8
to
1acfaec
Compare
Integrate with `<form>` elements directly through built-in support for [ElementInternals][]. This is achieved with graceful degradation in two ways: * automatically check for browser support * support globally disabling through `Trix.config.editor.formAssociated = false` According to the [Form-associated custom elements][] section of [More capable form controls][], various behaviors that the `<trix-editor>` element was recreating are provided out of the box. For example, the `<label>` element support can be achieved through [ElementInternals.labels][]. Similarly, a `formResetCallback()` will fire whenever the associated `<form>` element resets. Add support for integrating with [Constraint validation][] through the support for the `[required]` attribute and the `setCustomValidity(message)` method. [Constraint validation]: https://developer.mozilla.org/en-US/docs/Web/HTML/Constraint_validation [basecamp#1023]: basecamp#1023 [ElementInternals]: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals [Form-associated custom elements]: https://web.dev/articles/more-capable-form-controls#form-associated_custom_elements [More capable form controls]: https://web.dev/articles/more-capable-form-controls [ElementInternals.setFormValue]: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals/setFormValue [ElementInternals.labels]: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals/labels
1acfaec
to
5806006
Compare
static get formAssociated() { | ||
return config.editor.formAssociated | ||
} |
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.
@jorgemanrubia I've re-purposed this PR to re-introduce ElementInternals
alongside Constraint validation integration in a backwards compatible way.
Automatic support inference hinges on "ElementInternals" in window
. That assignment occurs in a new Trix.config.editor.formAssociated
value.
Is introducing a new configuration better than documenting that application code can assign Trix.elements.TrixEditorElement.formAssociated
directly?
Does the scope of this diff feel appropriate, or would it be better to split out a separate PR that introduces the configuration (or documents the Trix.elements.TrixEditorElement.formAssociated
property, if that feels better) to merge ahead of a separate PR to add constraint validation and [disabled]
support?
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.
Thanks for working on this @seanpdoyle. The scope and implementation are great. My only doubt is about exposing this as something you want to disable. In which scenarios would a Trix user want to disable this? I love that it will just disable it internally when not supported, but I wonder why would a customer want to disable that. I prefer not to add more user-facing config options unless justified.
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.
In which scenarios would a Trix user want to disable this?
I was considering the scenario you've outlined where an application's backwards compatibility commitments mean that they support browsers that do not support ElementInternals
.
If a team were supporting Safari 14, but rendered trix-editor[required]
or trix-editor[disabled]
, it'd behave as expected for users issuing requests from modern browsers, while those attributes would be ignored for the ones with legacy browsers.
If an application disabled the enhancements entirely, then their application could be forced to target the older browsers for support (by disabling any new functionality like Constraint validation).
I prefer not to add more user-facing config options unless justified.
If conditional support without user-facing doesn't feel risky, I'm happy to revert the configuration and CI seams.
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 reverted the Trix.config.editor
changes, and changed the guidance in the README.md
to encourage Trix.elements.TrixEditorElement.formAssociated = false
as the supported escape hatch.
0e686ba
to
7d3cf36
Compare
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.
Thanks for addressing the feedback @seanpdoyle. I left one last comment about the approach.
super() | ||
this.#internals = this.constructor.formAssociated ? | ||
this.attachInternals() : | ||
null |
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.
Instead of repeating the if/else structure in the corresponding methods, how do you feel about making that decision here and keep both paths separated. I haven't tested this but something like:
export default class TrixEditorElement extends HTMLElement {
constructor() {
// ...
if (!this.#internals) {
this.#disableElementInternals()
}
}
get disabled() {
return this.inputElement.disabled
}
#disableElementInternals() {
Object.assign(this, {
get disabled() {
console.warn("This browser does not support the [disabled] attribute for trix-editor elements.")
}
// ...
})
}
}
Open to other approaches, but I'd love to keep both paths separated: the default path where it assumes support and the alternative "not supported" path.
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 originally proposed #1132 to extract a delegate object to serve as a unified grouping of either the if
branch's code or the else
branch's code.
Knowing that object delegation in that style has fallen out of favor, I decided an explicit (albeit verbose) repetition of the conditional in each method would be easier to review.
Would something like this work:
class LegacyDelegate {
constructor(element) {
this.element = element
}
get disabled() {
console.warn("This browser does not support the [disabled] attribute for trix-editor elements.")
return false
}
// ...
}
class ElementInternalsDelegate {
constructor(element) {
this.element = element
this.internals = element.attachInternals()
}
get disabled() {
return this.element.inputElement.disabled
}
// ...
}
class TrixEditorElement extends HTMLElement {
static formAssociated = "ElementInternals" in window
#delegate
constructor() {
this.#delegate = this.constructor.formAssociated ?
new ElementInternalsDelegate(this) :
new LegacyDelegate(this)
}
get disabled() {
return this.#delegate.disabled
}
}
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 @seanpdoyle, that would work for me 👍
253dfcc
to
cff077e
Compare
Delegate implementation-specific methods and properties to the appropriate delegate. Set the delegate based on the `TrixEditorElement.formAssociated` property.
cff077e
to
1cfa094
Compare
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.
Thanks @seanpdoyle. This is a fantastic addition. I'll release a new version with it tomorrow.
@seanpdoyle could you have a look at the failing tests here https://github.com/basecamp/trix/actions/runs/11317691021/job/31471435810 please? Seems like a legit one. The sauce setup where it won't run on community PRs is certainly messy. |
@jorgemanrubia I've opened #1194. |
@jorgemanrubia what is likelihood that a release containing these changes can be incorporated into the release of Rails & Action Text 8.0? |
Yes @seanpdoyle. I'll release a new trix version today and will validate all is good in our apps, then I'll create a PR in Rails to bump actiontext. |
This updates Trix to 2.1.7, which includes support for the upcoming security fix in rails#51729 and enhanced form support via `ElementInternals` and HTML5 validations. See: - https://github.com/basecamp/trix/releases/tag/v2.1.7 - basecamp/trix#1190
This updates Trix to 2.1.7, which includes support for the upcoming security fix in #51729 and enhanced form support via `ElementInternals` and HTML5 validations. See: - https://github.com/basecamp/trix/releases/tag/v2.1.7 - basecamp/trix#1190
Follow-up to #1188
Follow-up to #1192
Closes #1023
Integrate with
<form>
elements directly through built-in support for ElementInternals.This is achieved with graceful degradation in two ways:
Trix.elements.TrixEditorElement.formAssociated = false
According to the Form-associated custom elements section of More capable form controls, various behaviors that the
<trix-editor>
element was recreating are provided out of the box.For example, the
<label>
element support can be achieved through ElementInternals.labels. Similarly, aformResetCallback()
will fire whenever the associated<form>
element resets.Add support for integrating with Constraint validation through the support for the
[required]
attribute and thesetCustomValidity(message)
method.Integrating with Element Internals
Trix will integrate
<trix-editor>
elements with forms depending on the browser's support for Element Internals. By default, Trix will enable support forElementInternals
when the feature is enabled in the browser. If there is a need to disable support forElementInternals
, setTrix.config.editor.formAssociated = false
:Validating the Editor
Out of the box,
<trix-editor>
elements support browsers' built-in Constraintvalidation. When rendered with the required attribute, editors will be
invalid when they're completely empty. For example, consider the following HTML:
Since the
<trix-editor>
element is[required]
, it is invalid when its valueis empty:
In addition to the built-in
[required]
attribute,<trix-editor>
elements support custom validation through their setCustomValidity method.
For example, consider the following HTML:
Custom validation can occur at any time. For example, validation can occur after
a
trix-change
event fired after the editor's contents change:Disabling the Editor
To disable the
<trix-editor>
, render it with the[disabled]
attribute:Disabled editors are not editable, cannot receive focus, and their values will
be ignored when their related
<form>
element is submitted.To change whether or not an editor is disabled, either toggle the
[disabled]
attribute or assign a boolean to the
.disabled
property:When disabled, the editor will match the :disabled CSS
pseudo-class.