WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 8 months ago

Last modified 8 months ago

#39763 closed defect (bug) (fixed)

W3C Error with get_calendar()

Reported by: Harry Milatz Owned by: isabel_brison
Milestone: 5.4 Priority: normal
Severity: minor Version: 4.7.2
Component: General Keywords: good-first-bug has-patch has-screenshots commit has-dev-note
Focuses: ui, accessibility Cc:

Description

Hi @all,

The W3C Validator reports an error:
Element tbody not allowed as child of element table in this context.

If the tbody tag will be added to

$calendar_output

before the tfoot tag is added, the error is gone.

Best regards,
Harry

Attachments (11)

general-template.php (125.7 KB) - added by Harry Milatz 4 years ago.
"correctet" file without produce the error
39763.diff (2.4 KB) - added by abhijitrakas 15 months ago.
39763.2.diff (2.5 KB) - added by isabel_brison 9 months ago.
Proposed change after a11y feedback from Nov 2019
39763.3.diff (2.5 KB) - added by audrasjb 9 months ago.
Adds i18n string and fix some WPCS issues
Capture d’écran 2020-01-23 à 10.17.14.png (36.9 KB) - added by audrasjb 9 months ago.
Before patch
Capture d’écran 2020-01-23 à 10.18.02.png (38.8 KB) - added by audrasjb 9 months ago.
After patch
39763.4.diff (1.7 KB) - added by sabernhardt 8 months ago.
adds classes to new elements, and removes "pad" class from empty #next span
39763.5.diff (1.8 KB) - added by audrasjb 8 months ago.
Widgets: Add CSS classes to calendar widget navigation
Capture d’écran 2020-02-21 à 00.13.40.png (39.6 KB) - added by audrasjb 8 months ago.
Testing 39763.5.diff
39763.6.diff (1.7 KB) - added by sabernhardt 8 months ago.
removes #prev and #next IDs
39763.7.diff (2.0 KB) - added by sabernhardt 8 months ago.
adds class to wp-calendar table

Download all attachments as: .zip

Change History (50)

@Harry Milatz
4 years ago

"correctet" file without produce the error

#1 @desrosj
17 months ago

  • Focuses accessibility added
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from normal to minor

Hi @harry-milatz,

My apologies that this ticket fell through the cracks! Are you able to create a proper patch with the suggested changes?

I just did some testing using the latest version of WordPress and Twenty Nineteen and it seems this issue still exists. After some searching, it seems that the <tfoot> element was allowed to precede the <tbody> element in HTML 5. However, that was changed in HTML 5.1 and <tfoot> must now follow <tbody>.

I am tagging this for accessibility review because moving the navigation links in <tfoot> below the table's body will likely have implications for users utilizing assistive technologies (a user may need to navigate through the entire calendar month even if it is not the desired month).

The implications this change has for CSS in themes must also be considered and tested to ensure no strangeness comes up.

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


17 months ago

#3 @abhijitrakas
15 months ago

  • Keywords has-patch added; needs-patch removed

@desrosj

I have made changes accordingly. Please check "39763.diff" file for changes. Please let me know if it needs any more changes.

Thanks,

@abhijitrakas
15 months ago

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


12 months ago

#5 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @SergeyBiryukov
12 months ago

  • Keywords commit added

Tested 39763.diff, looks good to me.

Tentatively marking for commit, pending feedback from the Accessibility team.

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


11 months ago

#8 @SergeyBiryukov
11 months ago

  • Keywords needs-patch added; has-patch removed

Per feedback from @joedolson in #accessibility:

the tfoot element should not contain controls - it should only contain <th> cells that act as headings. So, yes, I'd call that a problem.
The previous/next links really shouldn't be inside the table.

Looks like some investigation is needed on moving those links out of the table and how many themes having styles for the widget would be affected by that.

#9 @SergeyBiryukov
11 months ago

  • Keywords commit removed

#10 follow-up: @isabel_brison
10 months ago

Hey @SergeyBiryukov , I notice this ticket is assigned to you but also has needs-patch keyword; is it OK to pick up or are you working on it? Thanks!

#11 in reply to: ↑ 10 @SergeyBiryukov
10 months ago

Replying to isabel_brison:

I notice this ticket is assigned to you but also has needs-patch keyword; is it OK to pick up or are you working on it?

Feel free to pick it up, not actively working on it now :)

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


9 months ago

#13 @audrasjb
9 months ago

  • Owner changed from SergeyBiryukov to audrasjb
  • Status changed from reviewing to accepted

#14 @audrasjb
9 months ago

  • Owner changed from audrasjb to isabel_brison
  • Status changed from accepted to assigned

@isabel_brison
9 months ago

Proposed change after a11y feedback from Nov 2019

#15 @isabel_brison
9 months ago

  • Focuses ui added
  • Keywords has-patch added; needs-patch removed

I have added a patch that removes the <tfoot> altogether, and moves the "previous" and "next" month links into a <nav> that sits below the table. I couldn't think of a good way to semantically link this nav to the table, but they both sit under the same widget heading. Would be good to have some more accessibility feedback on this.

In terms of theme styling, this solution may look slightly broken with existing themes if they were depending on <tfoot> borders (e.g. Twenty Fourteen) or if they add generic styles to the <table> element (e.g Twenty Nineteen with the font family). Otherwise, for layout themes generally depend on the table layout itself, and that will be easy enough to replicate with a bit of CSS.

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


9 months ago

#17 @MarcoZ
9 months ago

I like that the table loses some of its layout table characteristic here and that the navigational links are in a proper nav element. One thing I noted is that the aria-label for the nav element needs to be localizable. Right now the string is hardcoded.

One question: While we're here, are there things in the thead that might also be more fitting outside the table?

@audrasjb
9 months ago

Adds i18n string and fix some WPCS issues

#18 @audrasjb
9 months ago

  • Keywords needs-dev-note added

Hi @isabel_brison thanks for the patch!

I refreshed the patch to add correct internationalization and to fix some minor WPCS issues with white spaces.

I tested the patch with bundled themes and it looks good to me.
Adding needs-dev-note keyword since theme authors may want to know about this change.

#19 @isabel_brison
9 months ago

@MarcoZ thanks for the feedback! The thead only contains the weekday as a heading for each of the columns, which I believe is semantically correct. There are no links in it. Does anything seem off to you?

@audrasjb thanks for fixing up the patch! Is it good to go now, or is there anything else I can do to progress this ticket?

#20 @audrasjb
9 months ago

  • Keywords commit added

Marking it for commit to get final review and commit action :-)

#21 @audrasjb
9 months ago

  • Keywords has-screenshots added

#22 @jorbin
9 months ago

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

In 47223:

General: Fix HTML for get_calendar()

The <tfoot> element was allowed to precede the <tbody> element in HTML 5. However, that was changed in HTML 5.1 and <tfoot> must now follow <tbody>.

This removes the tfooot and instead uses a nav element for navigation between months.

Fixes: #39763
Props: isabel_brison, Harry Milatz, audrasjb, abhijitrakas, SergeyBiryukov

#23 @jorbin
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because this needs a devnote.

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


9 months ago

#25 @audrasjb
9 months ago

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

Closing this ticket as fixed: it has needs-dev-note keyword and it's on our list for 5.4’s dev notes :)

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


9 months ago

#27 @BackuPs
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi Guys

Can you also add a unique class name to the nav wrapper, nav prev and nav next to make it easier to target them by css class name instead of by id and html element?

Thank you

Last edited 9 months ago by BackuPs (previous) (diff)

@sabernhardt
8 months ago

adds classes to new elements, and removes "pad" class from empty #next span

#28 @sabernhardt
8 months ago

  • Keywords needs-testing added; commit removed

This patch adds classes to the new nav and span elements:

<nav class="wp-calendar-months" ...>
<span id="prev" class="wp-calendar-previous-month">
<span id="next" class="wp-calendar-next-month">

I checked https://wpdirectory.net/ to make sure none of these names were already in one of those plugins or themes. If there's a better choice, though, feel free to change each name.

Note also: I expect that removing the "pad" class from the empty previous month's span tag was intentional. So I removed it from the empty next month's span as well. (Please correct if I'm wrong.)

#29 @audrasjb
8 months ago

Thanks for the suggestion @BackuPs and thank your for the patch @sabernhardt :-)

In 39763.5.diff I'd suggest to use the following classes, for better clarity:

  • .wp-calendar-nav for the navigation wrapper
  • .wp-calendar-nav-prev for the previous month link
  • .wp-calendar-nav-next for the next month link

Cheers,
Jb

@audrasjb
8 months ago

Widgets: Add CSS classes to calendar widget navigation

@audrasjb
8 months ago

Testing 39763.5.diff

#30 @audrasjb
8 months ago

  • Keywords commit added; needs-testing removed

Patch tested, it works fine for me.
@jorbin as the initial committer of the ticket, that would be wonderful if you were available to proof-review and hopefully commit this small patch to add classes to the navigation elements. Thanks!

#31 @sabernhardt
8 months ago

@audrasjb Thanks! I prefer your class names over mine.

#32 @jorbin
8 months ago

While we do that, I wonder if we should also remove the ids as it is possible multiple calendars could be used on one page. What do others think?

#33 @audrasjb
8 months ago

Looks like a great idea!

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


8 months ago

#35 @afercia
8 months ago

This ticket was discussed during today's accessibility bug-scrub: it would be great to have the patch updated as per last request to give this ticket a change to be included in 5.4.

@sabernhardt
8 months ago

removes #prev and #next IDs

#36 @sabernhardt
8 months ago

I briefly hesitated to remove the prev and next IDs because they are used by themes as selectors. However, if theme authors want to match the former styling, they probably need to update their stylesheets for the new elements anyway.

So it probably would be better to remove those IDs now.

#37 @sabernhardt
8 months ago

I'd like to propose adding one more class, to the table:
<table id="wp-calendar" class="wp-calendar-table">

Then that ID might be removed later (not yet, though, because thousands of themes use that to style the table).

@sabernhardt
8 months ago

adds class to wp-calendar table

#38 @SergeyBiryukov
8 months ago

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

In 47374:

General: Introduce HTML classes in get_calendar() for easier CSS targeting:

  • .wp-calendar-table for the <table> element.
  • .wp-calendar-nav for the navigation wrapper.
  • .wp-calendar-nav-prev for the previous month link.
  • .wp-calendar-nav-next for the next month link.

Replace #prev and #next HTML IDs with .wp-calendar-nav-prev and .wp-calendar-nav-next classes.

Props sabernhardt, BackuPs, audrasjb, jorbin.
Fixes #39763.

#39 @audrasjb
8 months ago

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

For information, the existing dev note was updated to reflect those enhancements.

Note: See TracTickets for help on using tickets.