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

RFC 60: Draftail Usage for General Text Entry #60

Merged
merged 5 commits into from
Sep 16, 2022

Conversation

jacobtoppm
Copy link
Member

@jacobtoppm jacobtoppm commented Sep 17, 2020

@jacobtoppm jacobtoppm changed the title Add mini draftail RFC Draftail Usage for General Text Entry Sep 17, 2020
@kaedroho kaedroho changed the title Draftail Usage for General Text Entry RFC 60: Draftail Usage for General Text Entry Nov 19, 2020
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

In addition to what the RFC covers at the moment, I’d like to see our plan for any areas where a <input type="text"> and <textarea> differ with Draftail. Thinking of two in particular:

max_length

If set, this will be used server-side for validation / storage, but browsers will also automatically truncate any text entry that goes beyond the maxlength. I presume we’d need to re-implement this – and give special consideration to Unicode gotchas like surrogate pairs so the counting is identical client-side and server-side. MDN says maxlength counts "UTF-16 code units".

This will be a bit annoying to have to reinvent but an upside is that we could also use this as an opportunity to introduce a character count for all fields in a way that’s consistent.

Accessibility

Visually I’m sure we can style Draftail to look almost identical to <input type="text"> and <textarea>. I wonder how big of a difference it would be for screen reader users though – I think it needs looking into. For the RFC, at least stating we intend for the fields to be announced similarly even if semantically they might be different. Worth considering how to convey a field is single-line vs. multiline for example? Checking what browsers might announce when maxlength is reached, if anything?

Other considerations

Things I wondered about and wasn’t sure whether they’d need looking into:

  • Support for browser extensions such as Grammarly or vim shortcuts that might work very well with native elements but not with our custom widget.
  • Reviewing the list of know Draftail issues for things that are relevant for plain text, and we might want to spend more time assessing if we used Draft.js for a lot more fields
  • How this RFC overlaps with Outline RFC 46: Single paragraph rich text fields/blocks #46

text/000-mini-draftail.md Show resolved Hide resolved
@thibaudcolas
Copy link
Member

thibaudcolas commented Nov 29, 2021

Adding further notes from my research:

Accessibility

  • The semantic differences between contenteditable and input and textarea aren’t necessarily too big as long as we use the textbox role, with appropriate ARIA attributes such as aria-multiline
  • The a11ysupport test suites for textarea and input type="text" can be reused when testing a textbox contenteditable
  • I couldn’t find any test suites & documented results for screen reader behavior on maxlen – checked a11ysupport and PowerMapper test suites.

max_length

  • The Python string len() used by Django’s MaxLengthValidator counts unicode characters
  • The JS string .length counts UTF-16 code points
  • According to MDN the maxlength also uses UTF-16 code points
  • It looks like the simplest way to count unicode characters in JS is to use the ES2015 unicode regex flag: 'my string'.match(/./ug).length.

@thibaudcolas
Copy link
Member

For people following this – the RFC is now tagged as Final Comment Period, as we are now considering implementing this as part of the upcoming Page editor 2022 project.

In particular, we’re considering:

  • Providing a visible character count and character length limit for all fields
  • Adding a sentence length visualisation mode to multiline fields. See for example https://github.com/wooorm/readability.

@lb-
Copy link
Member

lb- commented Mar 28, 2022

Put on the core team chat also.

My general concern is that we may be causing some known bugs in Draftail to now be propagated to all text fields.

As long as there is an easy opt in. (update. Originally wrote opt out)

springload/draftail#138
wagtail/wagtail#6631
wagtail/wagtail#4343
springload/draftail#260

I have not validated these myself - maybe they are fixed now.

@lb-
Copy link
Member

lb- commented Mar 28, 2022

We will also want to ensure that we support all native Dom events like blur/focus/click/change etc.

Personally I'm less convinced this is a good direction - we are heading towards native date picker for the reason that it's simpler, browser support is good and works without maintenance.

We may be creating something that adds a lot of overhead to simple fields in terms of complexity, reinventing the wheel and supporting all browsers and accessibility targets well.

As such I think this should be opt in only and be implemented like a custom widget.

@thibaudcolas
Copy link
Member

thibaudcolas commented Mar 30, 2022

@lb- contenteditable is a native browser feature – there is no way (that I’m aware of) to be more native if you want to support annotated markup in plain-text content. The most common alternative would be to have virtual field rendering with a <canvas> or other DOM rendering mimicking the input’s behavior, overlaid on top of the inputs / textarea, which has its own pitfalls.

@lb-
Copy link
Member

lb- commented Mar 30, 2022

Hey. Sorry what I meant was that we are potentially replacing plain text fields with a full JS driven react component right?

My concern is that we are making something the default that is more complex for simple text fields.

@lb-
Copy link
Member

lb- commented Apr 13, 2022

Draft.js has confirmed that it is no longer maintained. The project will no longer receive updates and the GitHub repo will be archived in October of this year (October 2022).

I think we need to consider this if we are to adopt our wrapper around Draft.js in even more places in Wagtail. This is frustrating I know but I personally do not think we can proceed with this RFC unless we have a more solid underpinning for a large change across basically any text field in Wagtail.

Maybe we can find a different way to solve for comments, feedback, and other features we want to implement around common fields.

facebookarchive/draft-js#3091 (comment)
facebookarchive/draft-js#3136

@thibaudcolas
Copy link
Member

This is unfortunate news and I think we should probably adjust our implementation of this RFC to be editor-agnostic where possible. Perhaps change the RFC itself to be completely editor-agnostic, and just say Wagtail’s rich text editor should be usable for fields saved as plain-text, and detail what that would look like.

Aside from this I don’t see how Facebook’s announcement makes Draft.js any less suitable of an underpinning, for this kind of feature, within Wagtail, in the mid term. We’ve been using Draft.js v0.10.5 since 4 years ago when we set it up without ever doing any upgrades, so although some of the known issues are annoying, they’ve never been annoying enough for us to really dig into. If we did have concerns about those bugs, we could still look into upgrading Draft.js (we have about 2.5 years of releases to catch up on), or fixing them outside the framework.


Re your previous comment @lb-,

My concern is that we are making something the default that is more complex for simple text fields.

If we want to support the features envisioned in the RFC, I don’t see any other way than to increase the complexity of plain-text fields. And regardless of which editor we use, I believe a JS-driven contenteditable is the least complex way to do this.

@thibaudcolas
Copy link
Member

thibaudcolas commented Sep 13, 2022

@jacobtoppm just coming back to all "Final Comment Period" RFCs – do you have thoughts about my suggestion above, to make this RFC editor-agnostic? Stipulate Wagtail’s rich text pipeline should support "plain text" fields, from rendering to editor widgets.

As we consider the future of rich text in Wagtail, I think it’d be better for us to get this kind of fundamental design choice decided on before we consider our options (investing in Draftail / Draft.js more, or fully focus on alternatives).

@jacobtoppm
Copy link
Member Author

@thibaudcolas I see what you mean. Should we move away from Draftail, we should have some other "rich" (for annotation/analysis type features) wrapper around plain text, which should probably be the new editor, yep, so I think it makes sense for at least some part of our rich text pipeline to be usable for plain text fields too. Most of the differences to this RFC would be on the FE/widget details side - essentially on the Django side we only need a widget that takes a string to render its initial data, and returns a string when converting back from POST data. The get_comments method mentioning isn't even essential I believe, since comment positions are currently saved via the comments form rather than the rich text field - it just provides a helpful way to access them, so could be worth removing to simplify this.

However, currently Draftail is where inline commenting is implemented, and that's a pretty big motivation for this. Performance testing was also done with Draftail only, so I don't think stripping the RFC down to make it completely generic adds that much? Maybe just adding another section/a prelude to say "this should apply whatever editor we choose; now here are the Draftail considerations"

text/000-mini-draftail.md Show resolved Hide resolved
text/000-mini-draftail.md Show resolved Hide resolved
text/000-mini-draftail.md Show resolved Hide resolved
text/000-mini-draftail.md Outdated Show resolved Hide resolved
@thibaudcolas
Copy link
Member

@jacobtoppm sounds reasonable to me! I’ve made edits towards this direction, let me know what you think.

jacobtoppm and others added 2 commits September 14, 2022 10:13
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
@jacobtoppm
Copy link
Member Author

Thanks @thibaudcolas, I agree with the suggestions - added them in.

@thibaudcolas thibaudcolas merged commit e18eedf into wagtail:main Sep 16, 2022
@thibaudcolas
Copy link
Member

thibaudcolas commented Sep 16, 2022

…aaand merged. There’s no plans for anyone to work on this in the short term, so still plenty of time for discussion if we have second thoughts. #8247 could be a good place to comment on, though I suspect we’re more likely to go ahead with #8249 (support for single-line rich text fields) than straight to plain-text "rich text" fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants