Rich text: remove space handling (for BUTTON and SUMMARY) #41977
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -77 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
fix/rich-text-space-legacy-api
branch
from
e1304d2 to
698a23e
Compare
ellatrix
changed the title
ellatrix
commented
Jun 27, 2022
| * | ||
| * DO NOT limit this behaviour to specific tag names! It would mean that this | ||
| * behaviour is not widely tested. If there's ever any problems, we should find | ||
| * a different solution entirely or remove it entirely. |
There was a problem hiding this comment.
It was a nice try, but this solution clearly failed. Cc @dmsnell
On my list but in my version I'm not using
Thanks for checking. I guess it could be possible that some integrations re-surface the issue with space not entering a space. For reference, I double-checked behaviors:
This was tested with a basic HTML file. <div contenteditable>This is content.</div>
<button contenteditable>This is content.</button>
<details>
<summary contenteditable>This can be edited</summary>
But this is static.
</details>I think we can probably handle this at those tags if and when it arises maybe. Both the button and summary have problems in edit mode where it's less important anyway that we use those specific tags (my argument is based on the claim that semantics while editing are different than when viewing). It could be best if we warn on |
dmsnell
approved these changes
Jun 27, 2022
There was a problem hiding this comment.
While this is still a draft I think it's a good call to gut it. I wish we had a good way to leave a tombstone in the code to say, "hey, we tried this and it failed" because I think the git history won't be easy to find. I don't have a good idea on how to do this so I'm not suggesting we block merging this until we find one, but if you have an idea it would be worth tossing it in I think.
Did you check with I guess another solution for I do think it's good to use the tags if possible in the future, because it's easier to style by themes. But anyway, it's not important right now. |
ellatrix
force-pushed
the
fix/rich-text-space-legacy-api
branch
from
698a23e to
5d882ec
Compare
No I didn't, because…
…anyway
My point about avoiding these tags is during the editing operation and I think all the styling concerns are diminished there. We know in the editor we have JavaScript and React and if we really want a collapsable editing container without the baggage of We don't have to insist on the same implementation ( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Seems to do more harm than good. We currently also don't have any blocks using rich text with BUTTON or SUMMARY as a tag name. Also work on the details block seems to be abandoned (#23940). There might be plugins using these tag names though...
Alternatively we could just return early for all other tag names.
Why?
How?
Testing Instructions
Screenshots or screencast