Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35021 closed defect (bug) (fixed)

Dashboard widgets doubled "Configure" link

Reported by: afercia's profile afercia Owned by: nicholas_io's profile nicholas_io
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: Widgets Keywords: has-screenshots good-first-bug dev-feedback has-patch
Focuses: ui, accessibility Cc:

Description

Noticed while investigating on #34987: the Dashboard widgets with a "Configure" link have two "Configure" links. The first one is inside the toggle button and even if it's visually hidden, it's still focusable and can be activated using a keyboard. The second one is inside the heading.

This is because the link HTML gets appended to the Widget name, see wp_add_dashboard_widget() and then the widget name is passed to add_meta_box() then in a global and finally used by do_meta_boxes(), both inside the toggle button and the heading. (phew!)

To check this you'd need to install a Plugin that provides a configurable Dashboard widget.

For now, in #34987 the first link gets hidden with CSS. A better solution would be avoiding to output useless markup in the first place.

Ideally, the widget name should be just a string with no HTML and the link output should use a separate variable but then a new argument would be required. To keep it simple maybe the HTML part of the string could be removed just before the output with explode() or something like that. A bit weak though. Open to suggestions :)

https://cldup.com/XEyW3OHe7f.png

Attachments (3)

35021.patch (2.0 KB) - added by nicholas_io 8 years ago.
35021.diff (3.3 KB) - added by swissspidy 8 years ago.
35021.2.diff (486 bytes) - added by swissspidy 8 years ago.
Fix error on menus screen

Download all attachments as: .zip

Change History (15)

#1 @swissspidy
8 years ago

  • Keywords needs-patch good-first-bug added

I've been playing with using wp_strip_all_tags() in do_meta_boxes for a bit.

It doesn't work too bad, but there are widgets that to some strange strings. The Quick Draft meta box for example has the following title:

<span class="hide-if-no-js">Quick Draft</span> <span class="hide-if-js">Drafts</span>

It would be more safe to allow people passing a separate title that can be used for that button to wp_add_dashboard_widget(). Example:

wp_add_dashboard_widget(
        'dashboard_quick_press',
        $quick_draft_title,
        'wp_dashboard_quick_press',
        null,
        array(
                'button_title' => 'test',
        )
);

The data in the $callback_args array can be checked for the existence of this title. If it doesn't exist, the widget title can be used as usual.

Just needs a better name instead of button_title, some docs and perhaps a review of what else this could be used for.

#2 @nicholas_io
8 years ago

Adding a new param like button_title is straightforward but it would depend heavily on developers using it, which is unlikely to happen since there's no visual change, "just" an accessibility improvement (unfortunately accessibility doesn't matter for most developers).

Another idea is to use $callback_args to add an internal/private param like __widget_basename in wp_add_dashboard_widget() and pass this param to the do_meta_boxes() function and check there if there's a $callback param called _widget_basename and use that value instead of the $widget_name and then unset this internal param do avoid passing it to the user callback function.

I've just attached a patch to implement this idea.

@nicholas_io
8 years ago

#3 @nicholas_io
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#4 @swissspidy
8 years ago

#36158 was marked as a duplicate.

@swissspidy
8 years ago

#5 @swissspidy
8 years ago

  • Milestone changed from Awaiting Review to Future Release

Thanks for the patch, @nicholas_io. That's a neat approach!

In 35021.diff I extended it to fix the same issue in the screen options as reported in #36158.

#6 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.6

Would be great if we could fix this in 4.6

#7 @DrewAPicture
8 years ago

  • Owner set to nicholas_io
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

#8 @ocean90
8 years ago

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

In 37972:

Dashboard: Don't add a "Configure" link to the toggle button.

The HTML for the toggle gets appended to the widget name which is later used for the widget title and the screen reader text of the toggle button. Storing the original widget name in the arguments allows us to use the name without the HTML for the screen reader text and doesn't require further changes by plugin developers.

Props nicholas_io, swissspidy.
Fixes #35021.

#9 @swissspidy
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because of #37298

PHP Fatal error: Cannot use object of type WP_Post_Type as array in .../wp-admin/includes/screen.php on line 123

#10 @swissspidy
8 years ago

#37298 was marked as a duplicate.

@swissspidy
8 years ago

Fix error on menus screen

#11 @swissspidy
8 years ago

  • Keywords has-patch added; needs-patch removed

35021.2.diff fixes the issue. Props @elrae for the patch (see #37298).

Apparently $box['args'] is always an object on the Menus screen. It just so happened that one of these objects has now turned into a proper WP_Post_Type class.

#12 @ocean90
8 years ago

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

In 38004:

Screen API: After [37972], ensure that $box['args'] is an array before trying to access __widget_basename.

This prevents a PHP fatal error on the Nav Menus screen where $args is an object.

Props elrae.
Fixes #35021.

Note: See TracTickets for help on using tickets.