fix(rich-text-input): fix for safari #1150
Conversation
montezume
force-pushed
the
ml-safari-fix
branch
from
3c22937 to
0e1e7c5
Compare
|
I would appreciate a quick review since this is a critical bug. cc @jonnybel |
| const children = React.cloneElement(next(), { | ||
| tagName: 'output', | ||
| id: internalId, |
There was a problem hiding this comment.
we don't want two elements with the same id, so we change the <div contenteditable> id to an internal id
| <HiddenInput | ||
| isFocused={isFocused} | ||
| handleFocus={editor.focus} | ||
| id={props.id} |
There was a problem hiding this comment.
the hidden input receives the "real" id, so that it can receive label focus
| @@ -129,6 +134,11 @@ const renderEditor = (props, editor, next) => { | |||
| return ( | |||
| <Editor editor={editor} {...passedProps}> | |||
| {children} | |||
| <HiddenInput | |||
There was a problem hiding this comment.
Is this like a focus trap?
There was a problem hiding this comment.
Can you explain what a focus trap is?
There was a problem hiding this comment.
It's a trap for the page's focus or the user's ability to tab around. So only any child within the focus-trap can be focussed. I wasn't sure if this was what we're doing here just couldn't derive fromt the name HiddenInput.
There was a problem hiding this comment.
To understand this, we need to step back.
When you write this in HTML, what does it mean?
<label htmlFor="name">Name</label>
It associates this label with the text "Name", with the <input> who's id is "name". Right?
<input>. Not div. Not span. You can't associate a label with a <div> or a <span>.
That's the problem.
Before we were not rendering a <div>, we were rendering an <output>.
You can associate labels with <outputs>
However, contenteditable outputs break in Safari.
The idea of the HiddenInput, is to allow users to specify label's with htmlFor.
Does that clear it up?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah got it. Thanks. So it's just the "label-anchor" or something like that.
Summary
Currently the
RichTextInputand theLocalizedRichTextInputare completely broken in Safari.After a lot of debugging, I figured out that combining
slate+contenteditable+<output>lead to this problem.In case anyone has forgotten, the reason why we wanted to use
outputin the first place, is because it natively supports working withlabels andhtmlFor.Instead, I added a hidden input and I manually focus the
RichTextInputbased on that.