WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 12 days ago

#25473 assigned defect (bug)

wp_text_diff creates wrong number of columns if title arguments are set

Reported by: joedolson Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.6.1
Component: Revisions Keywords: has-patch has-screenshots
Focuses: accessibility Cc:

Description

The 3rd parameter for wp_text_diff is an array with 3 possible arguments. If any of those arguments are provided, the thead element will contain 4 columns, but the text diff renderer has only produced 3 columns of data since version 3.6.

Patch revises wp_text_diff to generate only 3 columns.

Attachments (9)

25473.wp-text-diff.patch (758 bytes) - added by joedolson 6 years ago.
Patch to wp_text_diff
markup-table.diff (711 bytes) - added by mehulkaklotar 4 years ago.
th with colspan 2
Screenshot from 2015-07-31 12:22:33.png (14.1 KB) - added by mehulkaklotar 4 years ago.
td and th creates problem in markup and display
Revisions Screenshot 2019-05-11 at 14.20.21.jpg (215.7 KB) - added by afercia 5 weeks ago.
Users are informed there are 3 columns. The second one is empty. No clue what the first and third one are about.
revisions page headings wrong hierarchy.png (160.5 KB) - added by afercia 4 weeks ago.
25473.diff (13.5 KB) - added by afercia 4 weeks ago.
split view.png (31.4 KB) - added by afercia 4 weeks ago.
wp_text_diff() split view
inline view.png (30.5 KB) - added by afercia 4 weeks ago.
wp_text_diff() inline view
Revisions page.png (239.6 KB) - added by afercia 4 weeks ago.
Revisions page with patch applied

Download all attachments as: .zip

Change History (28)

@joedolson
6 years ago

Patch to wp_text_diff

#1 @c4xpl0siv3
6 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

Tested and fine

#2 @c4xpl0siv3
6 years ago

  • Keywords 2nd-opinion close added

#3 @joedolson
6 years ago

Can I ask how you tested this? Based on my check and code review, this is very clearlya bug, though it's not expressed in the built-in usage of revisions. I'm open to the possibility I'm missing something, but would appreciate knowing what I'm missing.

#4 @markoheijnen
6 years ago

  • Milestone Awaiting Review deleted

Clearing out the milestones for closed tickets on Awaiting Review

#5 @zodiac1978
4 years ago

  • Keywords close removed
  • Resolution worksforme deleted
  • Status changed from closed to reopened

I can reproduce this bug. Re-opening.

#6 @zodiac1978
4 years ago

If you omit the show_split_view argument there will be just one column in the table header which is also wrong and not documented in Codex.

See:
https://core.trac.wordpress.org/browser/tags/4.2.1/src/wp-includes/pluggable.php#L2325
and
https://codex.wordpress.org/Function_Reference/wp_text_diff

#7 @SergeyBiryukov
4 years ago

  • Milestone set to Awaiting Review

#8 @mehulkaklotar
4 years ago

When set title args, it is taking correct markup. But for the title_left and title_right. it should use colspan 2 to matchup with tbody td elements.

@mehulkaklotar
4 years ago

th with colspan 2

@mehulkaklotar
4 years ago

td and th creates problem in markup and display

#9 @mehulkaklotar
4 years ago

  • Severity changed from normal to major

#10 @mehulkaklotar
4 years ago

  • Keywords ui-feedback added

#11 @johnbillion
4 years ago

  • Keywords needs-testing added; 2nd-opinion ui-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from major to normal

#12 @SergeyBiryukov
3 years ago

#36127 was marked as a duplicate.

#14 @adamsilverstein
5 months ago

  • Milestone set to Future Release

Reopening for consideration since the current revisions screen is still in use until we can build a block based replacement for it.

#15 @jeremyfelt
5 weeks ago

  • Focuses accessibility added

Can we clarify what the expected markup here should be?

Right now, we have:

if ( $args['title_left'] || $args['title_right'] ) {
	$r .= "<tr class='diff-sub-title'>\n";
	$r .= "\t<td></td><th>$args[title_left]</th>\n";
	$r .= "\t<td></td><th>$args[title_right]</th>\n";
	$r .= "</tr>\n";
}

So if title_left and/or title_right is specified, we end up with the possibility of empty HTML tags: <td></td><th>....</th>.

The existing markup-table.diff patch on this ticket removes the empty <td> and adds a colspan to the <th> element instead. Is that an acceptable approach for semantics/accessibility? If so, then I'd propose we go for it.

FWIW: The core revisions interface does not use either of these arguments and instead creates another table above the wp_text_diff() output to display post titles when comparing.

:wave: @afercia for some input? :)

#16 @afercia
5 weeks ago

  • Keywords needs-refresh added; needs-testing removed

:wave: @jeremyfelt. First time looking into this, not sure the patch would solve the issue.

If I'm not wrong, in the split view WP_Text_Diff_Renderer_Table prints out 3 columns. Using two <th colspan='2'> would mismatch the columns count (4 vs. 3). However, seems to me this is not the only problem. Depending on the combination of the arguments, the table markup can be invalid in other cases too.

For example, when at least one of title_left or title_right is set, the <thead> element will contain 4 columns.

Also, empty <th> should be avoided: table headers are used to give table columns their name and can't be empty. Empty columns used only for spacing should be avoided as well. This is not just for the sake of proper markup, it's also because assistive technologies will announce to users incorrect information.

In the Revisions page, the table should have <th> headers, as there's no information about what the left and right column are about.

Overall, there's room for some good improvements. They require some refactoring. I've started working at a patch, hope to have some time in the next days to upload a first pass.

@afercia
5 weeks ago

Users are informed there are 3 columns. The second one is empty. No clue what the first and third one are about.

#17 @afercia
5 weeks ago

Also noticed show_split_view is undocumented and just saw you opened #47224 to address it :smile:

#18 @afercia
4 weeks ago

Also, noticed the headings hierarchy in the Revisions page skips one level. See attached screenshot.

@afercia
4 weeks ago

#19 @afercia
4 weeks ago

  • Keywords has-screenshots added; needs-refresh removed
  • Milestone changed from Future Release to 5.3

25473.diff is a first pass. Some of the things I'd like to propose:

In wp_text_diff():

  • removes the empty table columns
  • removes the <colgroup> and <col> elements: they were used only for styling: replaced with a CSS class
  • if $args['title'] is set, adds a table <caption> element
  • if title_left and title_right are set, adds table headers: if one of the two is empty, prints out an empty <td> element instead of an empty <th>
  • adds show_split_view to the default args

In the Revisions page:

  • some of the previous changes are repeated for the extra, custom, table used in this page
  • fixes the headings (hierarchy was wrong, see screenshot)
  • adds new, visually hidden, heading and help text
  • makes the slider "handles" a bit more accessible

Re: the table headers "Base revision" and "Compared revision": any suggestion for better wording and styling is welcome.

For testing, I used this snippet:

echo wp_text_diff(
	'My nice string',
	'My changed string',
	array(
		'show_split_view' => true,
		'title'           => 'Table Caption',
		'title_left'      => 'Left Title',
		'title_right'     => 'Right Title'
	)
);

Some testing would be nice. Please play with the parameters, change inline / split view, omit the title, etc. and check the table output is always valid, semantic markup.

Moving to 5.3 consideration.

@afercia
4 weeks ago

wp_text_diff() split view

@afercia
4 weeks ago

wp_text_diff() inline view

@afercia
4 weeks ago

Revisions page with patch applied

#20 @adamsilverstein
4 weeks ago

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