WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 3 months 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.4 Priority: normal
Severity: normal Version: 3.6.1
Component: Revisions Keywords: has-patch has-screenshots needs-design-feedback needs-design
Focuses: accessibility Cc:
PR Number:

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

Download all attachments as: .zip

Change History (45)

@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
5 years ago

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

I can reproduce this bug. Re-opening.

#6 @zodiac1978
5 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
5 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
4 years ago

#36127 was marked as a duplicate.

#14 @adamsilverstein
11 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
7 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
7 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
7 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
7 months ago

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

#18 @afercia
7 months ago

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

@afercia
7 months ago

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

wp_text_diff() split view

@afercia
7 months ago

wp_text_diff() inline view

@afercia
7 months ago

Revisions page with patch applied

#20 @adamsilverstein
7 months ago

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

#21 @adamsilverstein
4 months ago

25473.2.diff updates language and formatting slightly.

#22 @afercia
4 months 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
4 months 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
4 months 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
4 months ago

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

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


3 months ago

#27 @afercia
3 months ago

@adamsilverstein when you have a chance :) Anything left here?

#28 @adamsilverstein
3 months ago

@adamsilverstein when you have a chance :) Anything left here?

@afercia i'm a little hesitant about the visible labels for the added/removed columns. The appear disjointed from the design and I feel like we need more UX feedback here to get this right.

I'm going to try to refactor my the latest patch to remove that part and limit the scope to addressing the column issue originally raised in the ticket.

#29 @afercia
3 months ago

@adamsilverstein thanks for reviewing. Those "visible labels" aren't just text. They are, respectively:

  • $argstitle? -> <caption> element
  • title_left and title_right -> <th> table headers

As such they contribute to provide proper semantic information for the table. This is a data table and table headers are required to clarify what the data within the columns are. It's not just a visual thing :) and I wouldn't recommend to remove them.

No objections if you think a new design is necessary. However, the new design should respect web standards and accessibility constraints in order to provide semantic HTML and good level of accessibility (with visible table headers).

#30 @adamsilverstein
3 months ago

@afercia right, thanks for clarifying!

One issue is the headers appear twice currently, I feel like this should be a single table with a single set of headers. Ideally these headers would appear at the very top above the title.

The second issue is more visual: the headers do not look like headers, they might benefit from a background color or a larger, bolder font size. also some visual treatment, perhaps a border to more clearly define the columns. This is where I feel we could use some design input.

#31 @adamsilverstein
3 months ago

  • Keywords needs-design-feedback needs-design added

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


3 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


3 months ago

#34 @karmatosed
3 months ago

I do tend to agree to show twice feels not the most optimal visually, is there a way to avoid this?

As for styling, I feel making bold makes sense or some header treatment like h3/h4. I am unsure if having a header styling is counter to a11y benefits though, so saying 'like'.

#35 @adamsilverstein
3 months ago

  • Milestone changed from 5.3 to 5.4

Thanks for the feedback @karmatosed - I'll work on an update soon.

Punting to 5.4 since we are entering 5.3 beta and this isn't quite ready.

Note: See TracTickets for help on using tickets.