Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 23 months ago

#52224 closed enhancement (fixed)

RSS Widget: allow removing the feed icon link

Reported by: sabernhardt's profile sabernhardt Owned by: joedolson's profile joedolson
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-testing-info has-dev-note
Focuses: accessibility Cc:

Description (last modified by sabernhardt)

Removing the icon link can help fix at least these two bugs:

  1. #47670: Icon links for multiple widgets have identical link text.
  2. Hiding the icon without hiding/disabling the link creates an invisible link in themes such as Twenty Twenty-One (#52880).

On #47670, one possible solution was to remove the first link for everyone, but that would break probably all themes that float or hide the icon link with either .rsswidget:first-of-type or .rsswidget:first-child.

With a new filter, setting it to return false (or null) could solve both issues without breaking:
add_filter( 'rss_widget_feed_link', '__return_false' );
(Although, if a theme adds the filter and updates the styles accordingly, a child theme that replaces the parent stylesheet would likewise need updating to avoid any breakage.)

Another, separate possibility I've considered is including a new widget checkbox option so individual site owners can remove that link and/or include a link to the feed at the bottom of the widget instead. Then they can quickly add the link back if the layout breaks in their theme. If adding an option for the link within the heading, I'd like that to be unchecked by default, at least for any newly-added widgets. Ideally the heading would not contain both links.

Attachments (5)

52224.filter.diff (1.6 KB) - added by sabernhardt 4 years ago.
adding: filter, unique classes, internationalization, lazy loading
52224.2.diff (11.3 KB) - added by sabernhardt 3 years ago.
adding an icon display option to widget settings, updating unit tests, fixing lazy load attribute
52224.3.diff (12.9 KB) - added by sabernhardt 3 years ago.
twentytwentyonechild.zip (999 bytes) - added by sabernhardt 3 years ago.
child theme with two custom filter functions I tested
52224.4-filter.diff (8.1 KB) - added by sabernhardt 3 years ago.
filter with theme and unit test updates (no checkbox option), plus Twenty Seventeen RTL fix

Download all attachments as: .zip

Change History (37)

@sabernhardt
4 years ago

adding: filter, unique classes, internationalization, lazy loading

#1 @sabernhardt
4 years ago

  • Owner set to sabernhardt
  • Status changed from new to accepted

In addition to creating a filter for the icon link HTML, 52224.filter.diff:

  • Adds a unique class name to each of the links to help theme authors update styles with the :not() selector if necessary (example below is for Twenty Twenty)
    .widget_rss .widget-title a.rsswidget:first-of-type:not(.rss-widget-title) {
    	display: none;
    }
    
  • Adds internationalization function to translate "RSS" in alt text
  • Adds loading="lazy", as recommended on #50041
  • Moves the link-related variables inside the if($title) conditional

I expect this needs some polishing.

If desired, revisions to this filter could:

  1. Facilitate editing the heading text link as well (either adding an image inside or removing that link so the heading is only text).
  2. Encourage removing the icon link (within the docblock).
  3. Adjust the filter so we could consider a switch to opt-out later, if themes would not break by then.
Last edited 4 years ago by sabernhardt (previous) (diff)

#2 @sabernhardt
4 years ago

  • Keywords has-patch added

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


4 years ago

#4 @ryokuhi
4 years ago

  • Milestone changed from Awaiting Review to Future Release

Following feedback from the owner during the Accessibility Team's bugscrub today, moving to Future Release.

#5 @sabernhardt
4 years ago

  • Description modified (diff)

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


3 years ago

#7 @sabernhardt
3 years ago

  • Milestone changed from Future Release to 5.9

@sabernhardt
3 years ago

adding an icon display option to widget settings, updating unit tests, fixing lazy load attribute

#8 @sabernhardt
3 years ago

The RSS block does not include the icon (or the text heading), so this only affects the classic/legacy widget.

I added a checkbox setting for displaying the icon link in the latest patch. This should maintain the link (and automatically check the box) for any existing RSS widgets, but the checkbox should be unchecked when creating a new widget.

The patch now checks whether lazy loading is enabled before including the attribute. I also updated the unit tests' expected output.

#9 @audrasjb
3 years ago

That's a good patch idea :)
Shouldn't we show the icon by default to avoid introducing a breaking change?

#10 @sabernhardt
3 years ago

  • Keywords needs-refresh added

The main backward compatibility concern is making sure existing widgets keep the icon link, which the patch seems to do. However, most people who would create a new (legacy) RSS widget now probably would expect the link, so I'll switch the default checkbox value to checked. If we offer two methods to remove the icon, that is good enough.

Also, if a theme removes the link by setting the filter to return false, the checkbox option should not display in the widget's settings form.

@sabernhardt
3 years ago

#11 @sabernhardt
3 years ago

  • Keywords needs-testing added; needs-refresh removed

In addition to setting the checkbox checked by default and hiding that option when the filter is used, I included the bundled theme stylesheet edits.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#13 @Boniu91
3 years ago

Hello @sabernhardt Could you add steps that need to be performed in order to see the problem, and then to see the fix?

I was trying to reproduce it on the latest WordPress and Twenty Twenty-One but wasn't successful. As soon as the steps are here, we'll be able to test the ticket on our Test meetings.

#14 @sabernhardt
3 years ago

  • Keywords has-testing-info added

The patch for Twenty Twenty-One is separate, on ticket:52880 (if using the filter, the change here needs to be committed first). And to experience that bug, you need to navigate around the page with the Tab key and/or use a screen reader.

To make sure that the proposed checkbox option and/or filter do not cause new problems, you could follow the steps below.

Testing this patch

Create a legacy RSS widget before applying patch

  1. Install and activate the Classic Widgets plugin.
  2. Go to Appearance > Widgets (or Customize > Widgets) and add at least one RSS widget, with any combination of settings, to the most prominent sidebar area.
  3. Visit the site (front end), view source code, and record the current markup for the widget title (h2 heading).

Apply the patch

  1. Apply the latest patch. Clear any browser cache or open a different browser to compare both side-by-side.
  2. Visit the site and verify there are no errors on the front end. If the theme showed the feed icon before, the entire widget should appear the same now unless you are using Twenty Seventeen with an RTL language.
  3. View source code and record the updated markup for the widget title (h2 heading). It should have an rss-widget-feed class added to the first link and rss-widget-title on the second link. In some languages, the alt text could be something other than "RSS" now. If lazy loading is enabled, the loading="lazy" attribute should be included in the image markup.
  4. Also check the widget editor for error messages. (To see errors, you may need WP_DEBUG to be true in wp-config.php.)
  5. For testing 52224.4-filter.diff, skip ahead to the "Test the filter" section. Verify that the existing RSS widget's settings include a new "Display feed icon?" option and that this option is checked by default.

Create another legacy widget

  1. Add a new RSS widget after applying the patch, still with Classic Widgets activated.
  2. Verify that the "Display feed icon?" option is available and checked by default.
  3. Visit the site to verify that the feed icon shows in this new widget.
  4. Uncheck either widget's feed icon checkbox and click Save.
  5. Verify that the feed link is removed on the front end. The text link should appear the same as it was before removing the icon.

Test user settings in block widget editor

  1. While the Classic Widgets plugin is still active, uncheck the "Display feed icon?" option in one widget (if you have not done so yet).
  2. Create a new widget also and leave the default option(s) checked.
  3. Deactivate the Classic Widgets plugin.
  4. Visit the site to verify there are no errors and that the feed icon shows in this new widget.
  5. Go back to Appearance > Widgets.
  6. Verify that the newly-added RSS widget's settings include a new "Display feed icon?" option and that this option is checked by default. The other widget, with that option previously unchecked, should still have it unchecked.
  7. Uncheck the new feed icon checkbox and verify that the feed link is removed on the front end. The text link should appear the same as it was before removing the icon.

Test the filter

  1. Activate Twenty Twenty-One theme.
  2. Apply 52880.filter.patch. (Do not revert the other patch.)
  3. Go to Appearance > Widgets to make sure an RSS widget is in the desired sidebar area.
  4. Verify that the "Display feed icon?" option is not available.
  5. Visit the site, and view source code to verify that the feed link is not included in the widget heading. The text link should have the rss-widget-title class.
  6. Activate another theme.
  7. Use the same filter in a custom plugin (or child theme):
    add_filter( 'rss_widget_feed_link', '__return_false' );
  8. Again, check that the widget is in the desired sidebar area, verify that the option is not available, and check that the icon link is removed from the source code.
  9. If you would like, try creating a custom filter to change the output instead of removing it entirely. I'll upload a child theme with two custom filter functions that I tested, but the main purpose of the rss_widget_feed_link filter is to remove the link.
  1. Activate the Twenty Seventeen theme.
  2. Go to Appearance > Widgets to make sure an RSS widget is in the desired sidebar area.
  3. View the site and refresh the browser (clearing cache).
  4. Verify that the widget title text link is exactly the same, both with and without the icon link displayed. Note: 52224.4-filter.diff​ corrects the RTL stylesheet to make the icon link float left instead of to the right. Regardless of language, the icon link floats to the right when it shows. We could correct the RTL stylesheet to make it float left, but 52224.3.diff​ does not.
  5. Activate the Twenty Twenty theme.
  6. Go to Appearance > Widgets to make sure an RSS widget is in the desired sidebar area.
  7. View the site and refresh the browser (clearing cache).
  8. Verify that the widget title text link displays, either with or without the icon link in the markup. Even when the icon is there, it is hidden with CSS.
  9. Continue checking other themes, as desired.
Last edited 3 years ago by sabernhardt (previous) (diff)

@sabernhardt
3 years ago

child theme with two custom filter functions I tested

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#18 @Boniu91
3 years ago

@sabernhardt Thanks for detailed scenarios!

I'm having troubles with the Test user settings in block widget editor. Basically, it's not possible to uncheck the checkbox and save settings. Please check the following video:
https://jmp.sh/JWCh56S

Followed exactly the same steps starting from the top + used a couple of diferent themes.

Everything else is working properly.

#19 @sabernhardt
3 years ago

  • Keywords needs-refresh added; needs-testing removed

I thought I had fixed that by setting one of the values to 0 instead of 1:

   $show_icon    = isset( $widget_rss['show_icon'] ) ? (int) $widget_rss['show_icon'] : 0;

Before making that change, I had the same issue. Now, I am able to save the widget settings, but it sometimes reverts back when I refresh the (classic) Widgets page.

The problem is likely in the update and/or process functions, though I have not discovered it yet.

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


3 years ago

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


3 years ago

@sabernhardt
3 years ago

filter with theme and unit test updates (no checkbox option), plus Twenty Seventeen RTL fix

#22 @sabernhardt
3 years ago

  • Keywords needs-testing added; needs-refresh removed

I removed the unreliable checkbox option for the latest patch, and it also corrects the Twenty Seventeen RTL float direction.

The procedure above is updated, too.

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


3 years ago

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


3 years ago

#25 @Boniu91
3 years ago

Thank you @sabernhardt for detailed steps. I've followed them and got one question: am I right to say that the idea of checkbox is abandoned?

Test Report

Env

  • 5.9-alpha-51272-src
  • Theme: Twenty Twenty One, Twenty Seventeen, Twenty Twenty
  • Plugin: Classic Widgets

Tests summary

  1. Test the filter - Working correctly, returning false prevent icon from being displayed, filtering other values changes the display as expected.
  2. Verify site appearance does not change when removing an icon link - Working correctly on all mentioned themes, including RTL

#26 @sabernhardt
3 years ago

@Boniu91 Yes, the checkbox method is abandoned now. Thanks for testing (both times)!

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


3 years ago

#28 @joedolson
3 years ago

  • Keywords commit added; needs-testing removed
  • Owner changed from sabernhardt to joedolson
  • Status changed from accepted to assigned

#29 @joedolson
3 years ago

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

In 52031:

Widgets: Add filter to disable RSS widget icon.

Add a filter that disables output of the icon on RSS feed widgets. Improves accessibility by providing a path to prevent duplicate or invisible links.

Props sabernhardt, Boniu91.
Fixes #52224.

#30 @sabernhardt
3 years ago

  • Keywords needs-dev-note added; commit removed

#31 @sabernhardt
3 years ago

  • Keywords has-dev-note added; needs-dev-note removed

#32 @Mista-Flo
23 months ago

#50041 was marked as a duplicate.

Note: See TracTickets for help on using tickets.