WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 11 hours ago

#51445 closed task (blessed) (fixed)

[Multiple Bundled Themes] Add theme support for navigation-widgets

Reported by: Hareesh Pillai Owned by: williampatton
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.5
Component: Bundled Theme Keywords:
Focuses: accessibility Cc:

Description

#48170 introduced a theme support feature to wrap widgets displaying a lists of links within the <nav> element. All core WordPress themes should support this feature to improve semantics and accessibility.

Attachments (4)

51445.diff (2.5 KB) - added by Hareesh Pillai 4 weeks ago.
collage.png (196.5 KB) - added by Hareesh Pillai 4 weeks ago.
Comparison of the HTML structure before and after applying the patch for Twenty Twenty theme
navigation-widgets-screenshots.zip (5.6 MB) - added by sabernhardt 4 days ago.
screenshots of each theme from Twenty Thirteen to Twenty Twenty, plus a page for comparing before/after [49208]
51445.2.diff (838 bytes) - added by sabernhardt 4 days ago.
optional change for Twenty Fifteen and Twenty Sixteen

Change History (23)

@Hareesh Pillai
4 weeks ago

@Hareesh Pillai
4 weeks ago

Comparison of the HTML structure before and after applying the patch for Twenty Twenty theme

#1 @Hareesh Pillai
4 weeks ago

Twenty Thirteen and later themes have declared HTML5 support.
51445.diff adds navigation-widgets type to the html5 feature.

This ticket was mentioned in Slack in #accessibility by hareesh-pillai. View the logs.


4 weeks ago

#3 @justinahinon
4 weeks ago

51445.diff applies and works fine for the 2013 and later theme.

#4 @afercia
4 weeks ago

  • Milestone changed from Awaiting Review to 5.6

WordPress 5.5 introduced the navigation-widgets theme support, it would be nice to showcase this new feature in all the official themes at the next themes update.

To my understanding, bundled themes can now be released independently from core so I'm not sure which milestone would be the more appropriate for this ticket. Setting it to 5.6 for visibility.

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


2 weeks ago

This ticket was mentioned in Slack in #core-themes by hellofromtonya. View the logs.


2 weeks ago

#7 @williampatton
2 weeks ago

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

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


2 weeks ago

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


12 days ago

#10 @helen
12 days ago

  • Keywords needs-testing added

I don't think this is strongly tied to the 5.6 release process so I'm not going to punt for the moment but I would need a declaration from bundled themes maintainers that this a) doesn't break anything in those themes, and b) that this is okay to keep on their radar as they prep the bundled themes for 5.6 release.

#11 @SergeyBiryukov
12 days ago

  • Keywords commit added

See [48349] for some more context.

Looks like this only improves semantics and accessibility, but does not affect the appearance. Should be safe to commit after confirming.

#12 follow-up: @williampatton
12 days ago

I did a cursory scan over all the default themes for this in my test environment a few days back and there seemed to be no visual changes at all. I did not search the CSS of the themes to ensure there was no selectors that relied on there _not_ being an extra element: IE:

.widget-content > ul

#13 @SergeyBiryukov
12 days ago

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

In 49208:

Bundled Themes: Declare support for the html5 feature navigation-widgets.

Widgets that output a list of links are now wrapped within <nav> elements to improve semantics and accessibility.

The <nav> elements are native landmark regions, which helps assistive technology users to navigate through them.

Follow-up to [48349], [49177].

Props hareesh-pillai, justinahinon, afercia, williampatton.
Fixes #51445.

#14 in reply to: ↑ 12 @SergeyBiryukov
12 days ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to task (blessed)

Replying to williampatton:

I did a cursory scan over all the default themes for this in my test environment a few days back and there seemed to be no visual changes at all. I did not search the CSS of the themes to ensure there was no selectors that relied on there _not_ being an extra element: IE:

.widget-content > ul

Found this in Twenty Twenty:

.widget-content > div > *:first-child {
	margin-top: 0;
}

.widget-content > div > *:last-child {
	margin-bottom: 0;
}

Let's see if it needs any adjustment.

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


8 days ago

@sabernhardt
4 days ago

screenshots of each theme from Twenty Thirteen to Twenty Twenty, plus a page for comparing before/after [49208]

@sabernhardt
4 days ago

optional change for Twenty Fifteen and Twenty Sixteen

#16 @sabernhardt
4 days ago

  • Keywords needs-testing removed

There are no visible differences in my tests with Firefox (Windows), trying LTR and RTL languages and two screen/browser widths.

While Twenty Fifteen and Twenty Sixteen ul styles do have a bottom margin they didn't have before, the new margin does not increase the .widget container's greater bottom margin. If we want to remove this margin anyway, 51445.2.diff can do that for these two themes. Otherwise, this ticket could be re-closed, as is.

Twenty Twenty

There are three selectors involving the divs:
.widget-content > div > *:first-child
.widget-content > div > *:last-child
.widget_nav_menu .widget-content > div > ul

However, the widget's list styles have a margin of 0 (all directions) both before and after the change, due to this:

.widget_archive ul, .widget_categories ul, .widget_pages ul, .widget_meta ul, .widget_nav_menu ul, .widget_recent_comments ul, .widget_recent_entries ul, .widget_rss ul {
    list-style: none;
    margin: 0;
}

Also the tag cloud doesn't use the list tags in Twenty Twenty, so no change is necessary for that either.

Twenty Sixteen

The tag cloud list had a bottom margin and still does. The other widget lists now have a margin that the following used to override, though it's not noticeable.

.widget > :last-child {
    margin-bottom: 0;
}

Twenty Fifteen

Similarly to Twenty Sixteen, the tag cloud and most other navigation widget ul lists have a margin with the new markup. The RSS widget, however, retains the zero-margin:

.widget_rss ul {
	list-style: none;
	margin: 0;
}

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


23 hours ago

#18 @SergeyBiryukov
11 hours ago

In 49469:

Twenty Fifteen: Ensure the <nav> elements in widgets do not affect bottom margin.

Follow-up to [49208].

Props sabernhardt.
See #51445.

#19 @SergeyBiryukov
11 hours ago

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

In 49470:

Twenty Sixteen: Ensure the <nav> elements in widgets do not affect bottom margin.

Follow-up to [49208].

Props sabernhardt.
Fixes #51445.

Note: See TracTickets for help on using tickets.