WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 3 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 (11)

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 3 months 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 3 months ago.
25473.diff (13.5 KB) - added by afercia 3 months ago.
split view.png (31.4 KB) - added by afercia 3 months ago.
wp_text_diff() split view
inline view.png (30.5 KB) - added by afercia 3 months ago.
wp_text_diff() inline view
Revisions page.png (239.6 KB) - added by afercia 3 months ago.
Revisions page with patch applied
25473.2.diff (14.2 KB) - added by adamsilverstein 12 days ago.
Image 2019-08-12 at 6.13.20 PM.jpg (375.6 KB) - added by adamsilverstein 12 days ago.

Download all attachments as: .zip

Change History (35)

@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
7 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
4 months 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
3 months 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
3 months 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
3 months ago

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

#18 @afercia
3 months ago

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

@afercia
3 months ago

#19 @afercia
3 months 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
3 months ago

wp_text_diff() split view

@afercia
3 months ago

wp_text_diff() inline view

@afercia
3 months ago

Revisions page with patch applied

#20 @adamsilverstein
3 months ago

  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

#21 @adamsilverstein
12 days ago

25473.2.diff updates language and formatting slightly.

#22 @afercia
3 days ago

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

@adamsilverstein thanks for reviewing and adjusting! :) I'm not sure the new default left/right title are ideal. Maybe I'm missing something but:
'title_left' => __( 'Removed' )
'title_right' => __( 'Added' )

the left column may not necessarily have "removed" content, and the right one may not have "added" content. To my understanding, these are more "compare from" vs. compare to columns.

#23 @adamsilverstein
3 days ago

To my understanding, these are more "compare from" vs. compare to columns.

@afercia this is a common misunderstanding.

In fact the left column always shows what has been removed and the right column always shows what is added (in the current comparison). Some additional context is shown so you can tell where you are in the document, but the highlighted area is always additions on the right and deletions on the left.

You can prove this to yourself by creating a post and publishing, then add or remove one line and update, do this a few times then go to revisions and check what is shown. each time you remove a line, that change is shown in the left. when you add a line, that change is shown on the right.

This confusion has always bothered me, and I much prefer the single column view (show_split_view set to false)

#24 @afercia
3 days ago

@adamsilverstein thanks for clarifying! Yep, definitely it wasn't clear to me. Then, I guess the changes here would help also in making the UI clearer for everyone.

#25 @birgire
3 days ago

Related #47831 - regarding the UI of the "compare any two revisions"

Note: See TracTickets for help on using tickets.