Make WordPress Core

Opened 7 years ago

Last modified 3 years ago

#39909 reopened enhancement

Make title behaviours consistent across all widgets on first load

Reported by: karmatosed's profile karmatosed Owned by: westonruter's profile westonruter
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: has-screenshots 2nd-opinion a11y-placeholder needs-refresh needs-patch
Focuses: accessibility Cc:

Description

On first load the word 'Archives' outputs as the Widget title but it doesn't appear in the input field. If there is a default text, perhaps it should show. This could also assist by users making the connection with the words and where they can edit.

For example:

https://cldup.com/EK1v0wxGX1.png

Another example, the Calendar widget has no title and compared to the examples above this feels weird. What I think should happen is that the same title behaviour occurs for all widgets when you first load them.

Attachments (11)

39909.patch (9.9 KB) - added by bor0 7 years ago.
39909.2.patch (1.4 KB) - added by dlh 7 years ago.
39909.3.patch (7.4 KB) - added by bor0 7 years ago.
39909.4.patch (6.9 KB) - added by bor0 7 years ago.
39909.5.patch (6.9 KB) - added by bor0 7 years ago.
39909.6.diff (9.4 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/218
title-placeholders.mov (9.0 MB) - added by westonruter 7 years ago.
widgets-admin-screen-1.png (127.8 KB) - added by westonruter 7 years ago.
widgets-admin-screen-2.png (119.0 KB) - added by westonruter 7 years ago.
widgets-admin-screen-3.png (152.5 KB) - added by westonruter 7 years ago.
widgets-admin-screen-4.png (92.7 KB) - added by westonruter 7 years ago.

Change History (47)

#1 follow-up: @westonruter
7 years ago

  • Milestone changed from Awaiting Review to Future Release

@karmatosed what do you think about updating each widget to have a placeholder in each of their “Title” inputs if there is a default title that gets rendered in the preview? This would be a quick win.

Another example, the Calendar widget has no title and compared to the examples above this feels weird. What I think should happen is that the same title behaviour occurs for all widgets when you first load them.

In this case the placeholder would probably have to remain absent from the calendar widget, since there is no default widget heading. I don't think we'd be able to start introducing “Calendar” as a default widget title as this would be a backwards-incompatible change, such as for users who may be expecting the calendar widget to not have a heading when none is supplied. Does that make sense?

Aside: Placeholders were implemented for core widgets in the JS Widgets feature plugin.

#2 in reply to: ↑ 1 @karmatosed
7 years ago

Replying to westonruter:

@karmatosed what do you think about updating each widget to have a placeholder in each of their “Title” inputs if there is a default title that gets rendered in the preview? This would be a quick win.

Quick wins are 👍 I agree in hindsight lets not add a backwards-incompatible change.

#3 @westonruter
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Future Release to 4.7.4

Ready for dev. Patch would include whatever the default titles are as found in WP_*_Widget::widget() methods to be added to the title input placeholder in the correspondingWP_*_Widget::form() methods.

#4 @bor0
7 years ago

  • Keywords has-patch added; needs-patch removed

Added diff that implements the proposed behaviour.

@bor0
7 years ago

@dlh
7 years ago

#5 @dlh
7 years ago

@bor0 Your patch updates the various widget() methods, but I think the form() methods are what need updating? 39909.2.patch has an example of what I understood the intent to be.

#6 @westonruter
7 years ago

@dlh you are correct. 39909.2.patch is the direction to take.

@bor0
7 years ago

#7 @bor0
7 years ago

I see, thanks for the explanation! Added new patch that does that. I only updated widgets for which it made sense, i.e. widgets that also show their title by default.

Some examples that show their title (added placeholder): Archives, Categories, etc.
Some examples that don't show their title (didn't add placeholder): Text, Search, Calendar, etc.

#8 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed

@bor0 a couple points about 39909.3.patch:

  • The esc_attr_e() function, like the other translation functions, should not be passed a variable but rather a string. If you want to pass a variable, like $this->name, you should use echo esc_attr( $this->name ).
  • I'm not sure that it is right to print $this->name. I believe it should copy the string literal from the widget's widget method as @dlh showed in 39909.2.patch.
  • The change to class-wp-widget-tag-cloud.php is using a printing function in string concatenation, so that won't have the desired result.

#9 @bor0
7 years ago

@westonruter can quickly fix 1 and 3, but I'm wondering about 2. Shouldn't we change the widget method to use $this->name instead to keep consistency? In this case, it looks like we're repeating the same string 3 times (__construct, widget, form) which feels kind of weird.

#10 @dlh
7 years ago

The name property is documented as the "Name for the widget displayed on the configuration page." I took it to be only a coincidence that it was the same string as the default title.

Perhaps the duplication speaks to the need for something like a default_title property or $widget_options key? But, if so, I don't know whether that would be in-scope for this ticket.

Last edited 7 years ago by dlh (previous) (diff)

#11 @bor0
7 years ago

I agree about the scope changes part. If necessary, another ticket can be filed to take care of that.

@bor0
7 years ago

#12 @bor0
7 years ago

  • Keywords has-patch added; needs-patch removed

#13 @westonruter
7 years ago

@bor0 small change, but _e() in these cases should actually be esc_attr_e() to ensure the translated string gets escaped for an HTML attribute context.

#14 @bor0
7 years ago

Ah :) figured to use _e because we're using string literals but if that string changes in the future it can be an issue potentially. Submitting another patch in a few.

#15 @westonruter
7 years ago

@bor0 the problem isn't with the English string in core, but rather if the translated string from another language could potentially a character that would need to be escaped.

#16 @bor0
7 years ago

Ah. Good point. Thanks for the explanation!

#17 @bor0
7 years ago

@westonruter, I was searching for other appearances like that, and I found:

https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyseventeen/footer.php#L25
https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyseventeen/template-parts/navigation/navigation-top.php#L12

Do you think we should fix them as part of this ticket, or create another ticket for that?

@bor0
7 years ago

#18 @westonruter
7 years ago

@bor0 I added a couple more placeholders for dynamic defaults in 9126af5.

@karmatosed thoughts on title-placeholders.mov?

#19 @karmatosed
7 years ago

@westonruter, looks good to me!

#20 @westonruter
7 years ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted
  • Version set to 2.8

#21 @westonruter
7 years ago

  • Keywords has-screenshots added

#22 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 40251:

Customize: Show title input placeholders for widgets that have default titles when rendered.

Fixes #39909.
Props bor0, dlh, westonruter.

#23 @westonruter
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for consideration in 4.7.4

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#25 @afercia
7 years ago

  • Focuses accessibility added
  • Keywords 2nd-opinion a11y-placeholder added; commit removed

We've discussed a bit this change during today's accessibility meeting, and we wouldn't recommend to use the placeholder attribute for this purpose.

According to the spec, the placeholder attribute should be used to:

represents a short hint (a word or short phrase) intended to aid the user with data entry when the control has no value. A hint could be a sample value or a brief description of the expected format

Also, from a usability point of view, this change visually matches the placeholder with the widget title shown in the Customzer preview, but in the Widget screen doesn't help so much.

Since we're dealing with the default value, wouldn't be better to populate the field with the default value instead of using it as placeholder?

Adding an a11y-placeholder keyword since we're planning to use it for other cases related to placeholders.

Please, for any UI important change or changes to interactive controls, form fields, and so on, please always set the "accessibility" focus. 🙂

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


7 years ago

#27 @flixos90
7 years ago

  • Keywords needs-refresh added; fixed-major removed
  • Milestone changed from 4.7.4 to 4.8

I agree with @afercia here that providing the default values as actual values instead of placeholders would make more sense. It is not only better for accessibility, but they are actual values, not placeholders.

We could either show them initially (true default values), or we could consider to show them anytime the field is empty (also on subsequent actions of emptying a title). The latter would be a bit confusing, on the other hand the field being empty while the title shows up in the frontend doesn't make much sense.

Also given @swissspidy's comment in today's bug scrub, this ticket might not be significant enough to include it in 4.7.4.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#30 @westonruter
7 years ago

So how should we resolve this? Should the placeholder be moved to value? This would mean that the initial widget title in the widget in the preview will match the initial value in the title field when a widget is added, but if the user then empties out the field that value will actually still appear to persist, which would confuse users. To me it seems the placeholder would be less confusing in that regard.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#32 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed

Something else that I realized is that the placeholder or default value would need to be obtained with the frontend locale active. Currently it is using the admin locale, and so this could lead to an unexpected value shown if the admin and frontend have different locales.

And if we're not using placeholder either, it seems we need to revert this and revisit in the future. Nevertheless, the placeholder still seems like a better user experience (as opposed to default values) and reflective of the behavior of the behavior for an empty title on the frontend since they cannot be emptied.

@afercia @flixos90 @karmatosed so, revert or amend?

This ticket was mentioned in Slack in #core by westonruter. View the logs.


7 years ago

#34 @westonruter
7 years ago

In 40806:

Widgets: Revert [40251] pending more accessible solution for showing default widget titles rather than using placeholders.

See #39909.

#35 @westonruter
7 years ago

  • Milestone changed from 4.8 to Future Release

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.