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

Change font-size-picker markup to use select #16148

Merged
merged 6 commits into from Jul 9, 2019

Conversation

@tellthemachines
Copy link
Contributor

commented Jun 13, 2019

Description

Fixes #15319

Changed font-size-picker markup to use fieldset with legend as wrapper and replaced visual dropdown with select.

How has this been tested?

Tested locally on Chrome and on Safari with VoiceOver. Confirmed interaction with controls works as expected. Also tested on Windows with Firefox + NVDA; Noted NVDA doesn't read out the "Font size" legend before each field but only when entering the fieldset; VoiceOver reads it out before each field.

Wondering if it might be worth adding a snapshot and/or an e2e test for this.

Screenshots

Screen Shot 2019-06-13 at 5 53 07 pm

Screen Shot 2019-06-13 at 5 56 36 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

  • Changes to markup, css, state and value handling within font-size-picker component.
  • Changes to base-control and select-control components in order to allow control label to be visually hidden.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@tellthemachines tellthemachines marked this pull request as ready for review Jun 14, 2019

@tellthemachines tellthemachines requested review from nerrad and ntwb as code owners Jun 14, 2019

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@tellthemachines thanks for working on this!

From an accessibility perspective, everything looks good to me 👍

  • a native <select> element is very welcome
  • the fieldset legend makes sense when announced in combination with the input labels

As explained in related issues, it's important to consider the combination of the legend text and the various input labels. In this specific case, removing "font size" from the labels avoids redundant (and a bit pointless) information to be announced to users, as the group is already named "font size".

NVDA doesn't read out the "Font size" legend before each field but only when entering the fieldset; VoiceOver reads it out before each field.

Yep, implementations differ and actually the most correct behavior is the Firefox + NVDA one 🙂 The group name should be announced only when entering and exiting the group.

Note: the fieldset legend styling should be quickly tested with Internet Explorer 11, as I seem to recall it was a bit difficult to style in that browser.

The build fails, not sure why.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Thanks @afercia for the feedback! I changed the selectors on the e2e tests and now they are passing.

@tellthemachines tellthemachines requested a review from gziolo Jun 18, 2019

@tellthemachines tellthemachines requested a review from jorgefilipecosta Jul 2, 2019

@talldan

talldan approved these changes Jul 2, 2019

Copy link
Contributor

left a comment

This looks good to me. I had a few minor comments, but nothing major. Would be good to tidy up the comment above that import, but the other two comments are more subjective.

return (
<div className={ classnames( 'components-base-control', className ) }>
<div className="components-base-control__field">
{ label && id && <label className="components-base-control__label" htmlFor={ id }>{ label }</label> }
{ label && id && <label className={ classnames( 'components-base-control__label', hideLabelFromVision && 'screen-reader-text' ) } htmlFor={ id }>{ label }</label> }

This comment has been minimized.

Copy link
@talldan

talldan Jul 2, 2019

Contributor

This line is getting quite busy, it would be great to break it up onto multiple lines. The call to classnames could also be written like this, which is a pattern used more commonly across the codebase:

{ classnames( 'components-base-control__label', {
    'screen-reader-text': hideLabelFromVision,
} ) }
packages/components/src/font-size-picker/index.js Outdated Show resolved Hide resolved
@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@mapk @kjellr could I get your tick of approval (or design feedback) on this please?

Changes adressed

@karmatosed

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

If the decision is to use a select box then this works. I do wonder what the boundaries are on us styling that though, that's perhaps something to explore going forward. @afercia would you be able to advise on where those boundaries are?

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

If the decision is to use a select box then this works. I do wonder what the boundaries are on us styling that though, that's perhaps something to explore going forward. @afercia would you be able to advise on where those boundaries are?

I actually looked into this briefly a couple weeks ago. This seems to be mosly up to date:

https://css-tricks.com/dropdown-default-styling/

In general: Webkit browsers, and Firefox on Mac do not allow much styling. IE, Firefox, and Opera allow you to do a bit of minimal styling on Windows and Linux. (But I imagine that'll change a bit once the next version of IE adopts webkit.) Also, iOS does not allow you to style select fields. I do not have an Android phone handy at the moment, but I do not believe they support styling them either.

In short: if we move forward with a select box here, we shouldn't count on styling it.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

At Yoast we do use a component that's just a native styled select element. Basically, only the select "box" can be styled consistently across desktop browsers (including IE 11). The dropdown with options can't. Not sure about mobile browsers. Screenshots (Chrome on macOS - dark theme):

Screenshot 2019-07-08 at 15 24 20

Screenshot 2019-07-08 at 15 24 32

@enriquesanchez

This comment has been minimized.

Copy link

commented Jul 8, 2019

While I have never used nor tested it, this select component from Deque's patter library seems to be fully accessible while also allowing for custom styling: https://pattern-library.dequelabs.com/components/selects.

Could something like that work for us?

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

That looks like an excellent implementation. Still not native though. Off the top of my head, I can spot a couple issues at a first glance:

  • support for aria-haspopup="listbox" is still poor
  • to my knowledge, speech recognition software (Dragon) have troubles with aria-labelledby thus voice commands won't work

Maybe @dboudreau can help and give us some updated feedback or point us to someone at deque (when you have a chance, that would be greatly appreciated!). Background: evaluating to use the deque select component in WordPress.

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

A custom select component is worth exploring but out of scope for this issue. The decision in #15319 (comment) was to use a native <select>, so let's get this in as is and iterate! 😀

@noisysocks noisysocks merged commit 4912471 into master Jul 9, 2019

1 of 5 checks passed

Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@noisysocks noisysocks deleted the Fixes/Font-size-button-to-select branch Jul 9, 2019

@noisysocks noisysocks added this to the Gutenberg 6.2 milestone Jul 9, 2019

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Change font-size-picker markup to use select (WordPress#16148)
* Change font-size-picker markup to use select

* Improve naming.

* Fix e2e tests.

* Fix more tests.

* Address PR feedback.

* Address PR feedback.

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Change font-size-picker markup to use select (WordPress#16148)
* Change font-size-picker markup to use select

* Improve naming.

* Fix e2e tests.

* Fix more tests.

* Address PR feedback.

* Address PR feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.