Make WordPress Core

Opened 9 years ago

Last modified 3 years ago

#30556 accepted enhancement

Modern widgets default wrapper

Reported by: leopeo's profile LeoPeo Owned by: joedolson's profile joedolson
Milestone: Future Release Priority: normal
Severity: normal Version: 2.2
Component: Widgets Keywords: needs-patch
Focuses: accessibility, template Cc:

Description

Since 2.2.0, widgets are wrapped in <li></li> by default.
In my opinion, they should be wrapped in <div></div> even if, during the sidebar registration, theme developers should declare how to wrap the widgets in the sidebar.

Twentyten theme is the only default theme using this structure and it's declared in functions.php.

The patch contains this change.

Attachments (4)

new-widget-container.diff (572 bytes) - added by LeoPeo 9 years ago.
New widget container (<div> instead of <li>)
30556.diff (1.1 KB) - added by LeoPeo 9 years ago.
Widgets are wrapped in <aside> tag if theme supports html5 -> widget
30556.1.diff (608 bytes) - added by valendesigns 9 years ago.
30556.2.diff (595 bytes) - added by valendesigns 9 years ago.

Download all attachments as: .zip

Change History (59)

@LeoPeo
9 years ago

New widget container (<div> instead of <li>)

#1 @LeoPeo
9 years ago

  • Keywords has-patch added

#2 @obenland
9 years ago

  • Keywords needs-patch 4.2-early added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

Good idea! Changing the defaults like that is probably not very backwards compatible though.

We could use the HTML5 theme support construct to overwrite those defaults with aside tags, in case a theme adds support for HTML5 widgets. We could also use it to change the default widget heading class from widgettitle to widget-title.

#3 follow-up: @obenland
9 years ago

#26697 is an example of how we added HTML5 support to galleries, and could serve as a manual for this one.

#4 in reply to: ↑ 3 @LeoPeo
9 years ago

Thanks obenland for these tips!
i will work for a better patch!

@LeoPeo
9 years ago

Widgets are wrapped in <aside> tag if theme supports html5 -> widget

#5 @LeoPeo
9 years ago

  • Keywords needs-patch removed

30556.diff contains:

  • new default widget wrapper <aside> if theme declaresadd_theme_support('html5', 'widget');
  • changed heading class from widgettitle to widget-title

This makes default similar to Twentyfifteen sidebar.

#6 @LeoPeo
9 years ago

  • Keywords has-patch added

#7 @johnbillion
9 years ago

  • Version changed from trunk to 2.2

#8 @LeoPeo
9 years ago

Hi guys!
Any news from last patch submitted (30556.diff)?
Cheers!

#9 follow-up: @obenland
9 years ago

It's a step in the right direction, but adding the dash to the class name would again break backwards compatibility unfortunately.

I'd probably just check for HTML5 support after the initial defaults are set, and overwrite those three keys.

#10 in reply to: ↑ 9 @LeoPeo
9 years ago

Hi obenland, thanks for your reply!

It's a step in the right direction, but adding the dash to the class name would again break backwards compatibility unfortunately.

What do you mean?
I'm sorry but I don't understand what dash are you referring to...my fault :)

#11 @obenland
9 years ago

No worries. I'm speaking of the dash in "widget-title", on line 719 of your patch.

Themes that don't specify their own before_title argument will rely on .widgettitle for their CSS, so if we change that, their styles will break.

#12 @LeoPeo
9 years ago

Ok, I understand.
I believe, widgettitle class is annoying, but not blocking.
Any way, I agree this is a good opportunity to change it.

@valendesigns
9 years ago

#13 follow-up: @valendesigns
9 years ago

I've added a simpler and more backwards compatible patch to address this tickets concerns and move it forward.

Cheers,
Derek

#14 @obenland
9 years ago

  • Keywords 4.2-early removed
  • Milestone changed from Future Release to 4.2

#15 in reply to: ↑ 13 ; follow-up: @LeoPeo
9 years ago

Thanks valendesigns!

I've added a simpler and more backwards compatible patch to address this tickets concerns and move it forward.

I like this solution, still I'm not convinced about the title class.

For sure, this solution is more backward compatible, but the title class change needs to be well documented.
As theme developer, I don't expect this change declaring add_theme_support('html5', 'widget'); and this can happen if I want to update the widget structure of my wordpress.com theme.

#16 in reply to: ↑ 15 ; follow-up: @valendesigns
9 years ago

Replying to LeoPeo:

Thanks valendesigns!

I've added a simpler and more backwards compatible patch to address this tickets concerns and move it forward.

I like this solution, still I'm not convinced about the title class.

For sure, this solution is more backward compatible, but the title class change needs to be well documented.
As theme developer, I don't expect this change declaring add_theme_support('html5', 'widget'); and this can happen if I want to update the widget structure of my wordpress.com theme.

This is already the established pattern and one we should be following here. If you look at how other html5 support has been added in the past you will see that the classes are renamed. For example, look at how get_search_form supports html5 and you will see that the classes are manipulated. It's to be expected.

I don't believe, beyond saying what the defaults are, that any other documentation is needed to explain the class name change. That's because they don't yet exist and we're deciding what it should be in this ticket. It should not come as a shock to anyone that not only does the markup change, so does the class.

#17 in reply to: ↑ 16 @LeoPeo
9 years ago

Replying to valendesigns:
Ok, for sure someone with more experience in core development then me knows how to act in this situation :)

#18 @valendesigns
9 years ago

  • Keywords dev-feedback added

#19 @obenland
9 years ago

  • Keywords commit added; dev-feedback removed

30556.1.diff looks good.

If a theme author adds support for HTML5 widgets, it's not too much to ask to know what it does. Adding HTML5 support to WordPress features also lets us do things in a new way without having to worry about backwards compatibility. Additions are usually announced on make/core as well.

#20 @valendesigns
9 years ago

@obenland Should I write up a quick post to highlight the changes or is that something the core devs do?

#21 @obenland
9 years ago

Go for it!

#22 @valendesigns
9 years ago

Sweet! Doing it now. Should I just post it when I'm finished or is there some submission process I should be aware of?

#23 @valendesigns
9 years ago

@obenland Is it cool if I borrow some of the text you wrote in https://make.wordpress.org/core/2014/04/15/html5-galleries-captions-in-wordpress-3-9/ about forwards compatibility, as it's still relevant here?

#24 @obenland
9 years ago

Sure, go ahead.

#25 @valendesigns
9 years ago

I've created a draft post on make/core to explain this change, and I'll submit it for review once this patch is added to the core.

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


9 years ago

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


9 years ago

#28 @DrewAPicture
9 years ago

@obenland: What's your take on this? Do we need more time to soak or would you advise committing it anyway and seeing what happens in beta?

#29 @DrewAPicture
9 years ago

  • Owner set to ocean90
  • Status changed from new to reviewing

Looks pretty good for commit as we head into 4.2 Beta 1.

#30 @ocean90
9 years ago

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

In 31729:

Introduce HTML5 widgets support.

When a theme supports HTML5 widgets via add_theme_support( 'html5', 'widgets' ), aside will be used instead of list markup.

props LeoPeo, valendesigns.
fixes #30556.

#31 @ocean90
9 years ago

Note: In [31729] I've changed widget to widgets, plural. See https://wordpress.slack.com/archives/core/p1426105788003232.

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


9 years ago

#33 @joedolson
9 years ago

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

After seeing the announcement at make/core, the accessibility team discussed this issue (https://wordpress.slack.com/archives/accessibility/p1426180904001542) and we're concerned about the use of aside to wrap all widgets. From an accessibility perspective, this creates a massive profusion of landmarks that could make landmark navigation pretty well useless. An aside carries the implicit role of 'complementary', so it'll show up for assistive technology as a landmark location. Given that many themes use widgets as ways to add a huge variety of information - sidebars, mega footers, headers, front page managers, etc., this could mean that a given page might have dozens of landmarks.

We'd rather see this marked up using <section>, which doesn't carry that navigational semantic, so that landmarks can be reserved for larger regions of the page.

@valendesigns
9 years ago

#34 @valendesigns
9 years ago

  • Keywords has-patch added; needs-patch removed

I've added patch 30556.2.diff that changes aside to section. I will also update the blog post when it's been committed to reflect the change in markup.

#35 @valendesigns
9 years ago

  • Keywords dev-feedback added

Any update on when this will be committed? I need to update the Make Core blog post about this feature and the longer it sits here the longer the wrong information is being presented to the community.

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


9 years ago

#37 follow-up: @obenland
9 years ago

  • Keywords dev-feedback removed

I agree, section would be more appropriate. On top of the landmark issue, asides are meant to be related to the content around them, which would not be the case in the majority of widget areas.

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


9 years ago

#39 @ocean90
9 years ago

In 31826:

Revert [31729] since <aside> seems not to be the appropriate HTML5 tag.

see #30556.

#40 @ocean90
9 years ago

  • Keywords needs-patch added; commit has-patch removed
  • Milestone changed from 4.2 to Future Release

#41 @greenshady
9 years ago

<section> makes the most sense for the reasons mentioned above. It's what I've always used for widgets since switching to HTML5. FWIW, this was debated at length by several devs when we made this the default in the Hybrid Core theme framework. Ultimately, we came to the same conclusion as @joedolson and @obenland.

#42 in reply to: ↑ 37 @mrwweb
9 years ago

Replying to obenland:

I agree, section would be more appropriate. On top of the landmark issue, asides are meant to be related to the content around them, which would not be the case in the majority of widget areas.

The final HTML5 specification of the aside element allows for a varying definition of "related" based on whether the aside is nested in an article or not. (See also: http://html5doctor.com/aside-revisited/, http://stackoverflow.com/questions/8407805/best-html5-markup-for-sidebar)

I believe then that this suggests aside would make an excellent sidebar wrapper with each widget wrapped with a section element for themes with html5 support as suggested by @joedolson.

#43 @cais
9 years ago

I can see using <aside> as a general "sidebar" wrapper but can also see some possible issues as noted by @joedolson due to its "landmark" uses for a11y; and, I definitely can see the <section> element as a functional widget wrapper ... again for the same reasons already noted, such as those by @greenshady.

#44 @tradesouthwest@…
9 years ago

Aside is not appropriate for sidebars. Most all sidebars do not contain 'article' or 'section', 'header', 'article' content that is "aside" the article content. i think everyone is mislead on the standard of this sematic. Aside for a widget is far from appropriate and would not pass standards as to the correct use of 'aside'.

I will never use aside semantic for a sidebar as it has nothing to do with "aside" the content. Please refer to HTML5 reference to get this right.

<article>
<p>
As of writing, the only web browser completely support date time input is Opera.
In HTML5, it is the job of web browser to ensure user can only enter a valid date time string
into the input textbox.
</p>

<aside>
Picking a date from Calendar is not the only way to input a date value even though it's the most popular implementation.
HTML5 specifications does not mention anything about displaying a calendar for date input.
</aside>
</article>

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


9 years ago

#46 follow-up: @Otto42
9 years ago

The aside element appears to be absolutely appropriate for sidebars. They even show it being used just that was as an example in the HTML5 specification:

http://www.w3.org/TR/html5/sections.html#the-aside-element

Look down to "The following extract shows how aside can be used for blogrolls and other side content on a blog".

I am uncertain as to the use in accessible browsers, but the specification is very clear here. What's the issue, exactly? What is landmark navigation and how does it relate to this issue?

Version 0, edited 9 years ago by Otto42 (next)

#47 @tradesouthwest@…
9 years ago

Maybe aside will go away just like hgroup did. It seems like a non-used attribute as it is. Not really a comfortable element in a already-complex environment.

#48 in reply to: ↑ 46 @afercia
9 years ago

Replying to Otto42:

What is landmark navigation and how does it relate to this issue?

Please install a screen reader. I'd recommend NVDA to be used with Firefox. Then, actually use it. Then, please, read again the specs :) "aside" elements are mapped to "role=complementary".
http://www.w3.org/TR/html5/sections.html#the-aside-element
http://www.w3.org/TR/wai-aria/roles#complementary
Add, say, 10 widgets into your sidebar. You don't want to hear (example from Twenty Fifteen theme):

complementary landmark 
search landmark 
SEARCH
button
complementary landmark 
RECENT POSTS
heading  level 2
complementary landmark 
RECENT COMMENTS
heading  level 2
complementary landmark 
ARCHIVES
etc. ...

that's because each widget is currently an "aside" and thus mapped to "complementary" role. Instead, the widget area should be one, single, aside. Currently, we have just too many landmarks.
Please consider screen readers allow users to navigate through landmarks using specific keystrokes, in NVDA you can do that just pressing "D". See: http://www.w3.org/WAI/GL/wiki/Using_ARIA_landmarks_to_identify_regions_of_a_page
Additionally, worth considering not all widgets are really "tangentially related" to the main content. (calendar? search?).

#49 @Otto42
9 years ago

I'll give that a try, but, regarding this:

Additionally, worth considering not all widgets are really "tangentially related" to the main content. (calendar? search?).

Widgets are always, in some way, related to the page-as-a-whole. Asides are only related to their immediate parent. If you have asides in the body, then they're related to the body, not necessarily to the articles which would presumably be the main content area for posts and such.

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


8 years ago

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


7 years ago

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


5 years ago

#55 @afercia
5 years ago

  • Milestone set to Future Release

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


3 years ago

#57 @joedolson
3 years ago

  • Owner set to joedolson
  • Status changed from reopened to accepted
Note: See TracTickets for help on using tickets.