Updates to the Font Size Picker #9802

mtias · 2018-09-11T18:21:42Z

This PR aims to add a bit more clarity and simplify the font-size control. It removed the S M L XL denotations in favor of size reference. It also groups the custom size input with the reset and on the same line. Furthermore, it disabled the display of the slider by default.

Sizes are rendered at their correct value (as supplied by the theme). "Normal" is the same as "Reset".

In Progress

To Do:

Use NavigableMenu?.
Figure out what components should exist here (Select based on Dropdown?)

davewhitley · 2018-09-12T01:18:54Z

Does the size progression of the 'A' hinder accessibility? Seems like now, the UI relies on vision.

mtias · 2018-09-12T10:08:02Z

Does the size progression of the 'A' hinder accessibility? Seems like now, the UI relies on vision.

The buttons have tooltips for "Small", "Medium", "Large".

mtias · 2018-09-25T14:18:12Z

I'm thinking of revisiting this and going with a selector like this:

Scales better to what the theme supplies (more options) and is arguably clearer than iconography.

mtias · 2018-09-28T14:12:21Z

@jasmussen should we make the input match the height of the select now?

jasmussen · 2018-09-28T14:20:05Z

Yes I think so.

mcsf · 2018-10-03T18:29:21Z

Pushed 6a4f92d.

Worth noting that navigating in a <NavigableMenu /> breaks in Safari when using ArrowUp. See also this mention in Slack.

This commit does a few things: 1. It makes the panel heading the same color as the down chevron. 2. It adds a label to the font size picker, grouping it all together 3. It tightens the margins between switch and contextual help text. 4. It relaxes the margins for base controls inside panels, to make them a bit lighter. Together with #9784 it reduces the weight of panels in the sidebar.

- Add "huge" and "normal" variants. - Normal equals a "reset".

afercia · 2018-10-03T22:06:38Z

packages/components/src/font-size-picker/index.js

 	return (
-		<Fragment>
+		<BaseControl
+			label={ __( 'Font Size' ) }


The BaseControl renders a <label> element with a for id that is not associated to any element. It's an orphaned label which basically doesn't do anything. So, the visible text "Font Size" shouldn't be a <label> element.

The BaseControl renders a <label> element with a for id that is not associated to any element. It's an orphaned label which basically doesn't do anything. So, the visible text "Font Size" shouldn't be a <label> element.

@afercia Noting this after my comment at #9802 (comment) . If I understand correctly, the issue was more that a label element was being rendered, but without the necessary for attribute? Am I correct in thinking that there should either be a for attribute assigned, or no label (i.e. text) at all? As merged, the implementation was changed to render a span in place of the label, which doesn't feel quite right to me when there's clearly an input related to the label's text.

Yep I've just commented on #10925 (comment) using the same screenshot :D

When the font size picker was changed to a custom component, the label was an orphaned label.

Am I correct in thinking that there should either be a for attribute assigned

The size picker custom component is made of buttons so a label with a for attribute wouldn't be appropriate.

or no label (i.e. text) at all?

I guess the text is necessary for visual purposes.

For the records: I wasn't happy with the new size picker, it's non-native and its semantics and interaction are problematic. That change was introduced without even asking for some initial accessibility feedback. Alas, this is what happens when visual design considerations take priority on semantics and accessibility.

Could the label element have directed to the button rendered by the custom dropdown (what appears in the screenshot above as "Normal" dropdown-looking element)?

Separately, considering how we might help avoid this scenario in the future: It would be fairly trivial to create an ESLint rule which forbids a BaseControl that includes label prop but omits id. One problem with this may be, as you point out, that a visual text may be appropriate as long as the inputs assign aria-label? Scanning those child elements would be more difficult to enforce. Arguably in those scenarios I might suggest the visual label be rendered as just another child, not using the label prop. That way, we can support both cases and ensure that label prop, if present, must have a corresponding id.

By above, what I mean is that both of these would be valid:

<BaseControl label="Font Size" id="my-input"> <input id="my-input" /> </BaseControl>

<BaseControl> <span>Font Size</span> <input aria-label="Font Size" /> </BaseControl>

And the following invalid (a case very similar to #10925):

<BaseControl label="Font Size"> <input id="my-input" /> </BaseControl>

I might suggest the visual label be rendered as just another child, not using the label prop.

Seems the most appropriate thing to do!

afercia · 2018-10-03T22:09:52Z

packages/components/src/font-size-picker/index.js

-				</ButtonGroup>
+					) }
+					renderContent={ () => (
+						<NavigableMenu aria-labelledby={ labelId }>


~~I don't see this aria-labelledby rendered in the DOM, so the whole menu is unlabelled.~~
Ah ok I see now it's rendered on the menu when it's open. However, this is a problem as when the menu is closed, the toggle button will just say, for example, "Normal" without telling what it's about.
This design is using an UI control to communicate a setting state, and not the available action. As mentioned several times this is an anti-pattern for a11y.

afercia · 2018-10-03T22:14:45Z

packages/components/src/font-size-picker/index.js

+						className="components-range-control__number"
+						type="number"
+						onChange={ onChangeValue }
+						aria-label={ __( 'Custom Size' ) }


I'd suggest to clarify and use "custom font size". Also, since this is invisible to the eyes, it doesn't need to be title cased.

packages/components/src/font-size-picker/index.js

afercia · 2018-10-03T22:23:02Z

Worth noting that navigating in a breaks in Safari when using ArrowUp. See also this mention in Slack.

In Safari, I see the arrows just triggers WritingFlow in the selected block. In Chrome, I've seen the page was scrolling but can't reproduce consistently. I guess the event propagation should be stopped.

afercia · 2018-10-03T22:27:07Z

packages/components/src/font-size-picker/index.js

+					renderContent={ () => (
+						<NavigableMenu aria-labelledby={ labelId }>
+							{ map( fontSizes, ( { name, size, slug } ) => (
+								<Button


As the NavigableMenu uses a role="menu", all the menu items within it should have a role="menuitem".

It'd be great to have a lint rule that checks for role="menu" in children of any NavigableMenu.

youknowriad · 2018-10-04T18:13:43Z

Not certain how to reproduce the Safari bug, for me Up/down arrows only navigate the font picker.

aduth · 2019-02-15T14:16:52Z

packages/components/src/base-control/index.js

@@ -7,7 +7,8 @@ function BaseControl( { id, label, help, className, children } ) {
 	return (
 		<div className={ classnames( 'components-base-control', className ) }>
 			<div className="components-base-control__field">
-				{ label && <label className="components-base-control__label" htmlFor={ id }>{ label }</label> }
+				{ label && id && <label className="components-base-control__label" htmlFor={ id }>{ label }</label> }
+				{ label && ! id && <span className="components-base-control__label">{ label }</span> }


The use-case does not feel legitimate. I don't think we should support BaseControl with a label and not an id. The label should always direct to an input, even in the case of the font size picker.

This leaves open the possibility for error like described at https://github.com/WordPress/gutenberg/pull/10925/files#r257248594 .

mtias added the [Status] In Progress Tracking issues with work in progress label Sep 11, 2018

kjellr mentioned this pull request Sep 21, 2018

Add a hover state to sidebar panel headers #10070

Merged

mtias force-pushed the update/font-picker branch from b470109 to 4bc0b28 Compare September 25, 2018 14:19

mtias added this to the 4.0 milestone Sep 28, 2018

mtias and others added 9 commits October 3, 2018 15:37

Rework font-size selection to improve clarity of options and the UI.

713526e

Tweak the reset button, make it disabled until a number is input.

3e7c044

Fix reset clearing input value.

3ab378d

Use different sizes for buttons and render.

af3dfc3

Convert font picker to a dropdown select showing real visual sizes.

132680f

- Add "huge" and "normal" variants. - Normal equals a "reset".

Remove unusued CSS code.

7848506

FontSizePicker: a11y: Wrap with NavigableMenu

5f2da9a

e2e: Update to accommodate new font size picker

18ba6db

mcsf force-pushed the update/font-picker branch from 6a4f92d to 18ba6db Compare October 3, 2018 20:19

Amend editor-modes test

5cf9607

afercia reviewed Oct 3, 2018

View reviewed changes

packages/components/src/font-size-picker/index.js Show resolved Hide resolved

afercia reviewed Oct 3, 2018

View reviewed changes

Accessibility improvements to the FontSizePicker component

62cb488

mcsf approved these changes Oct 4, 2018

View reviewed changes

youknowriad merged commit e5ed1b1 into master Oct 4, 2018

youknowriad deleted the update/font-picker branch October 4, 2018 18:25

This was referenced Oct 7, 2018

Update Font Size Selector #10387

Closed

Some parts of the layout break with longer translations #9537

Closed

dimadin mentioned this pull request Oct 15, 2018

i18n: Mark custom font sizes labels as translatable #10615

Merged

jorgefilipecosta mentioned this pull request Oct 18, 2018

Fix: Font size default classes. #10719

Merged

m-e-h mentioned this pull request Nov 6, 2018

Match height of font range control with the size select. #11555

Merged

4 tasks

aduth reviewed Feb 15, 2019

View reviewed changes

aduth mentioned this pull request Feb 15, 2019

Focal Point Picker #10925

Merged

4 tasks

This was referenced Feb 24, 2019

FocalPointPicker renders unlabelled input fields #13894

Closed

BaseControl should require an id or create one if missing #6989

Closed

aduth mentioned this pull request Mar 1, 2019

Code quality: Remove unnecessary BaseControl usage from video block #14179

Merged

afercia mentioned this pull request May 5, 2019

Custom interactions in control styled as a native control #15319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to the Font Size Picker #9802

Updates to the Font Size Picker #9802

mtias commented Sep 11, 2018 •

edited

Loading

davewhitley commented Sep 12, 2018

mtias commented Sep 12, 2018

mtias commented Sep 25, 2018

mtias commented Sep 28, 2018

jasmussen commented Sep 28, 2018

mcsf commented Oct 3, 2018

afercia Oct 3, 2018

aduth Feb 15, 2019 •

edited

Loading

afercia Feb 15, 2019

aduth Feb 15, 2019

afercia Feb 15, 2019

afercia Oct 3, 2018 •

edited

Loading

afercia Oct 3, 2018

afercia commented Oct 3, 2018

afercia Oct 3, 2018

mcsf Oct 4, 2018 •

edited

Loading

youknowriad commented Oct 4, 2018

aduth Feb 15, 2019

Updates to the Font Size Picker #9802

Updates to the Font Size Picker #9802

Conversation

mtias commented Sep 11, 2018 • edited Loading

davewhitley commented Sep 12, 2018

mtias commented Sep 12, 2018

mtias commented Sep 25, 2018

mtias commented Sep 28, 2018

jasmussen commented Sep 28, 2018

mcsf commented Oct 3, 2018

afercia Oct 3, 2018

Choose a reason for hiding this comment

aduth Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

afercia Feb 15, 2019

Choose a reason for hiding this comment

aduth Feb 15, 2019

Choose a reason for hiding this comment

afercia Feb 15, 2019

Choose a reason for hiding this comment

afercia Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

afercia Oct 3, 2018

Choose a reason for hiding this comment

afercia commented Oct 3, 2018

afercia Oct 3, 2018

Choose a reason for hiding this comment

mcsf Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

youknowriad commented Oct 4, 2018

aduth Feb 15, 2019

Choose a reason for hiding this comment

mtias commented Sep 11, 2018 •

edited

Loading

aduth Feb 15, 2019 •

edited

Loading

afercia Oct 3, 2018 •

edited

Loading

mcsf Oct 4, 2018 •

edited

Loading