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

Fix placeholder positioning #1477

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Fix placeholder positioning #1477

merged 1 commit into from
Jun 27, 2017

Conversation

ellatrix
Copy link
Member

Alternative fix for #1247.

  1. Makes sure boxes are properly wrapped around the placeholder.
  2. "Inherits" styling. (Not really, but has same classes and tag.)
  3. Does not use :before or :after, so no style conflicts.
  4. Editable is easily focusable is empty state.
  5. Caret stays in the middle.

Yay! (I think)

@jasmussen
Copy link
Contributor

Hmm. Impressive! Seems to work well for me. 👍 👍

@ellatrix
Copy link
Member Author

It's so silly! Don't know why I didn't think about this earlier.

Added an aria-label too.

@ellatrix ellatrix force-pushed the try/placeholder-pos branch 2 times, most recently from f998488 to a3c2142 Compare June 27, 2017 08:13
@ellatrix
Copy link
Member Author

Fixed some multi-line bugs.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Seems to work as intended. Nice work 👍

className="blocks-editable__tinymce"
style={ style }
>
{ inline ? placeholder : <p>{ placeholder }</p> }
Copy link
Member

Choose a reason for hiding this comment

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

Curious the a11y impact of having placeholder both as aria-label of the TinyMCE and as the content of the adjacent placeholder node. Should this be aria-hidden perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! What are your thoughts, @afercia?

key={ key }
/>
{ this.state.empty &&
<Tagname
className="blocks-editable__tinymce"
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should include a modifier class here so reading the stylesheet is more obvious the intention on the .blocks-editable__tinymce + .blocks-editable__tinymce styling.

@afercia
Copy link
Contributor

afercia commented Jul 3, 2017

I'm not sure I'm able to fully follow 🙂 not so familiar with this part and with what inline Editables are. One important thing to consider is that contenteditable regions tend to be reported by screen readers as sort of form fields. The behavior differs across screen readers but they understand it's an editable region. Thus, an aria-label gets announced as it was the label of a form field:

screen shot 2017-07-03 at 11 54 58

NVDA instead tends to announce as editable the actual HTML element, for example it announces:
paragraph editable for the text block
list editable for the list block
text frame editable for the button block (because it's a span)

and it does announce the aria-label. So what happens, in a few words?
There's the risk the visible placeholder text is reported as content of the editable text, so hearing:
Write label ... Write label should be avoided. Hiding it with aria-hidden seems a good idea.

However, I'm not sure we should use Write label… or Write… as aria-label in the first place. They behave like labels of a form field so they will be announced even after users enter some content, for example:

screen shot 2017-07-03 at 11 55 35
screen shot 2017-07-03 at 11 56 01

This could be confusing, as the aria-label text in this scenario is not so appropriate. Wondering how to improve the overall way editable regions are exposed to assistive technologies, will open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants