WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 6 weeks ago

#39909 reopened enhancement

Make title behaviours consistent across all widgets on first load

Reported by: karmatosed Owned by: westonruter
Milestone: 4.8 Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: has-patch has-screenshots 2nd-opinion a11y-placeholder needs-refresh
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 8 weeks ago.
39909.2.patch (1.4 KB) - added by dlh 8 weeks ago.
39909.3.patch (7.4 KB) - added by bor0 8 weeks ago.
39909.4.patch (6.9 KB) - added by bor0 8 weeks ago.
39909.5.patch (6.9 KB) - added by bor0 8 weeks ago.
39909.6.diff (9.4 KB) - added by westonruter 8 weeks ago.
https://github.com/xwp/wordpress-develop/pull/218
title-placeholders.mov (9.0 MB) - added by westonruter 8 weeks ago.
widgets-admin-screen-1.png (127.8 KB) - added by westonruter 7 weeks ago.
widgets-admin-screen-2.png (119.0 KB) - added by westonruter 7 weeks ago.
widgets-admin-screen-3.png (152.5 KB) - added by westonruter 7 weeks ago.
widgets-admin-screen-4.png (92.7 KB) - added by westonruter 7 weeks ago.

Change History (38)

#1 follow-up: @westonruter
2 months 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
8 weeks 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
8 weeks 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
8 weeks ago

  • Keywords has-patch added; needs-patch removed

Added diff that implements the proposed behaviour.

@bor0
8 weeks ago

@dlh
8 weeks ago

#5 @dlh
8 weeks 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
8 weeks ago

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

@bor0
8 weeks ago

#7 @bor0
8 weeks 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
8 weeks 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
8 weeks 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
8 weeks 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 8 weeks ago by dlh (previous) (diff)

#11 @bor0
8 weeks ago

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

@bor0
8 weeks ago

#12 @bor0
8 weeks ago

  • Keywords has-patch added; needs-patch removed

#13 @westonruter
8 weeks 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
8 weeks 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
8 weeks 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
8 weeks ago

Ah. Good point. Thanks for the explanation!

#17 @bor0
8 weeks 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
8 weeks ago

#18 @westonruter
8 weeks ago

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

@karmatosed thoughts on title-placeholders.mov?

#19 @karmatosed
7 weeks ago

@westonruter, looks good to me!

#20 @westonruter
7 weeks ago

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

#21 @westonruter
7 weeks ago

  • Keywords has-screenshots added

#22 @westonruter
7 weeks 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 weeks 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 weeks ago

#25 @afercia
7 weeks 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.


6 weeks ago

#27 @flixos90
6 weeks 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.

Note: See TracTickets for help on using tickets.