WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#31315 closed defect (bug) (fixed)

_navigation_markup doesn't allow themes/plugins to customize template.

Reported by: joedolson Owned by: obenland
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.1
Component: Themes Keywords:
Focuses: accessibility, template Cc:

Description

While the default template may be excellent for many themes, the use of an additional landmark role generated by the <nav> element and the specific use of an H2 element can lead to problems.

The problems related to two specific issues: first, a profusion of minor navigation groupings referenced by landmarks can make navigation using landmarks less useful. The specifications for the <nav> element note that "the element is primarily intended for sections that consist of major navigation blocks" (http://www.w3.org/TR/2011/WD-html5-author-20110705/the-nav-element.html). While for a given site, this may constitute a major navigation element (in purpose, if not in number of links), it's certainly not going to be the case for all.

The second issue relates to headings hierarchies - the H2 heading may not fit with the needs of a given design for providing an accurate hierarchy.

This is ultimately a very minor change; simply allowing authors the possibility of changing the default templates in this function.

Attachments (3)

31315.patch (745 bytes) - added by joedolson 4 years ago.
Add filter to _navigation_markup template.
31315.2.patch (724 bytes) - added by mordauk 4 years ago.
31315.diff (2.5 KB) - added by obenland 4 years ago.

Download all attachments as: .zip

Change History (23)

@joedolson
4 years ago

Add filter to _navigation_markup template.

#1 @joedolson
4 years ago

See also: #30589 (comments)

#2 @obenland
4 years ago

  • Component changed from General to Themes
  • Milestone changed from Awaiting Review to Future Release

Yes, we should absolutely make that filterable, that was the idea from the get go.

I think we should wait with making it filterable until #30589 is in though, to avoid surprising theme authors with using the snippet in a different context (comments vs. posts).

Thoughts?

#3 @joedolson
4 years ago

Do you think it makes a difference whether #30589 is in before making the shared function filterable? I'm not sure I can see why that would make a difference; it won't affect anything unless somebody adds a filter.

We might want a parameter made available to the function that provides context, though; I included $class in the filter as something that would be relatively easy to work with, but it's not explicitly a context indicator.

#4 @obenland
4 years ago

I like the idea of a context parameter! How about posts, post, and comments?

#5 @joedolson
4 years ago

Those make sense as values for now; I can't think of another context this is likely to come up in, at least in the short term.

I can refresh the page on Saturday, when I get back from conferencing...

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


4 years ago

@mordauk
4 years ago

#7 @mordauk
4 years ago

31315.2.patch fixes the missing variable assignment for the filter.

#8 @obenland
4 years ago

#32226 was marked as a duplicate.

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


4 years ago

#10 @joedolson
4 years ago

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

#11 @obenland
4 years ago

  • Milestone changed from Future Release to 4.4
  • Version set to 4.1

#12 @wonderboymusic
4 years ago

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

In 33714:

Add a filter to _navigation_markup: 'navigation_markup_template'

Props joedolson, mordauk.
Fixes #31315.

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


4 years ago

#14 @obenland
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#15 @obenland
4 years ago

  • Keywords has-patch removed
  • Owner changed from joedolson to obenland
  • Status changed from reopened to accepted

This will likely need more work with #30589 depending on it as well.

#16 @DrewAPicture
4 years ago

In 33717:

Docs: Add a description and example to the hook docs for the navigation_markup_template filter, introduced in [33714].

Since the value of the filter is passed through sprintf() it's important to note that any filtered output needs to contain the expected specifiers.

See #31315.

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


4 years ago

@obenland
4 years ago

#18 @obenland
4 years ago

I worked on a patch to add a context parameter to the filter (31315.diff), and it ended up just replacing the $class variable. The more I look at it the more I keep thinking we might as well just keep it as it is now, and continue to use the class name to provide context as it currently uses the same identifier with a -navigation suffix.

Thoughts?

#19 @joedolson
4 years ago

Yes, I agree. It doesn't really seem like changing to a context variable adds very much. It's fundamentally the same thing, so it seems pretty redundant.

#20 @obenland
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.