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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add back role textbox to the List block and improve aria-multiline usage #7432

Merged
merged 4 commits into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@afercia
Contributor

afercia commented Jun 21, 2018

Description

As discussed during WCEU contributor day, the best option for #5983 is to make the list block have the semantics of a textarea. This PR:

  • adds back role=textbox to the list block editable element
  • simplifies the condition, no more reason to use indexOf() as we're now checking only against a "table" tagName
  • also, after #7306 ensures the aria-multiline attribute is used only when there's a role=textbox. According to the spec, this attribute can be used only with a textbox role so it should be omitted for the table element instead of being false

@jorgefilipecosta the last point works but maybe can be improved, please do feel free to jump in if you have a chance, thanks! 馃檪

How has this been tested?

  • npm test and updated snapshots

Fixes #5983

@afercia afercia requested a review from jorgefilipecosta Jun 21, 2018

ariaProps.role = 'textbox';
} else {
// The `aria-multiline` attribute must be used only together with `role=textbox`.
ariaProps[ 'aria-multiline' ] = null;

This comment has been minimized.

@youknowriad

youknowriad Jun 21, 2018

Contributor

I think we discussed that we should almost always use aria-multiline. So what if we remove it entirely from the ariaProps and just add it here where we add ariaProps.role = 'textbox';. This seems consistent for me?

This comment has been minimized.

@afercia

afercia Jun 21, 2018

Contributor

Yep, we missed the table case... 馃槵 Still, we should allow to use both true (emulating a textarea) and false (emulating an input field). Omit it entirely when there's no role=textbox.

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Agreed on this one, we should be setting properties rather than removing it 馃憤

This comment has been minimized.

@afercia

afercia Jun 21, 2018

Contributor

Sure, agreed to simplify and always handle aria-multiline internally setting it to true as TinyMCE always behaves like a textarea, where text naturally wraps in multiple lines (regardless of the Enter key behavior). Removed the prop from the readme doc.

@youknowriad

馃憤 Nice

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 21, 2018

Thanks for the help!

@afercia afercia merged commit 9065b38 into master Jun 21, 2018

2 checks passed

codecov/project 46.81% (+<.01%) compared to 5ea93cd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/list-block-role-textbox branch Jun 21, 2018

@youknowriad youknowriad added this to the 3.1 milestone Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment