wp_text_diff creates wrong number of columns if title arguments are set

[joedolson]

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.

[joedolson]

Patch to wp_text_diff

[c4xpl0siv3]

Tested and fine

[joedolson]

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.

[markoheijnen]

Clearing out the milestones for closed tickets on Awaiting Review

[zodiac1978]

I can reproduce this bug. Re-opening.

[zodiac1978]

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

[mehulkaklotar]

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]

th with colspan 2

[mehulkaklotar]

td and th creates problem in markup and display

[SergeyBiryukov]

#36127 was marked as a duplicate.

[adamsilverstein]

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

[jeremyfelt]

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? :)

[afercia]

: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]

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

[afercia]

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

[afercia]

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

[afercia]

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]

wp_text_diff() split view

[afercia]

wp_text_diff() inline view

[afercia]

Revisions page with patch applied

[adamsilverstein]

25473.2.diff​ updates language and formatting slightly.

[afercia]

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.

[adamsilverstein]

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)

[afercia]

@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.

[birgire]

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

[afercia]

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

[adamsilverstein]

@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.

[afercia]

@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).

[adamsilverstein]

@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.

[karmatosed]

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'.

[adamsilverstein]

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.

[melchoyce]

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.

Agreed — these updates make an already confusing screen way harder for me to understand.

[audrasjb]

Hi there,
WP 5.4 Beta 1 is planned for tomorrow so let's move this ticket to 5.5. @adamsilverstein, of course, feel free to move it back to 5.4 if there is a patch for this ticket :-)

Cheers,
Jb

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (​https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: ​https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

• FAQs for New Contributors: ​https://make.wordpress.org/core/handbook/tutorials/faq-for-new-contributors/
• Contributing with Code Guide: ​https://make.wordpress.org/core/handbook/contribute/
• WordPress Coding Standards: ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/
• Inline Documentation Standards: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/
• Browser Support Policies: ​https://make.wordpress.org/core/handbook/best-practices/browser-support/
• Proper spelling and grammar related best practices: ​https://make.wordpress.org/core/handbook/best-practices/spelling/

-->

Trac ticket:

[melchoyce]

@adamsilverstein Can you add screenshots of your patch's change, either here or in the PR?

[adamsilverstein]

@melchoyce yep, will bring this up again and add some screenshots. Thanks!

[davidbaumwald]

@adamsilverstein Is this still on your list to tackle in 5.5?

[adamsilverstein]

I've been focused elsewhere and haven't had a chance to revisit this one yet. Punting to future release until myself or someone else can pick it up again.

Trac ticket:

[adamsilverstein]

Refresh against trunk in 25473.3.diff​

Here are some screenshots as you requested @melchoyce - I tested with trunk to document the current state/issue:

I added some css border lines to the table (table.diff td{ border: 1px dotted lightgray; }) so the issue is more apparent.

​https://share.getcloudapp.com/o0umkyvN

Using the filter and adding a title:
​https://share.getcloudapp.com/wbuPJ1P8

Adding only title_left: (missing column) ​https://share.getcloudapp.com/8Lurdm5G

Also problematic with table_right:
​https://share.getcloudapp.com/X6udYW8k

Show split screen false by default looks good:
​https://share.getcloudapp.com/5zuwDWPm

Adding the title or left/right messes the display up:
​https://share.getcloudapp.com/bLuwJ6yr

​https://share.getcloudapp.com/bLuwJ6Qr

I'm going to review and review the current patch to see if fixes all these layouts, I think the work diverged here to addressing the removed/added column confusion which is a somewhat separate issue.

[paaljoachim]

Does this ticket still need design and/or design feedback?

[karmatosed]

@paaljoachim yes it does, are you up for diving in there? The latest screenshots need a review.

[adamsilverstein]

After reviewing this again with @paaljoachim I think the best thing would be to apply the simplest solution possible, such as that suggested in https://core.trac.wordpress.org/attachment/ticket/25473/markup-table.diff

This fixes the original error reported in the ticket ("wp_text_diff creates wrong number of columns if title arguments are set") without growing the scope. I'd love to see a revisions feature re-imagined for the block editor and include lessons we have learned here. I'll mark this for 5.7 and do more testing.

[ryokuhi]

Reassining to @joedolson after short discussion during today's weekly accessibility bug-scrub.

[estelaris]

Design team will wait for accessibility review on this ticket. If our feedback is still needed, we will review again.

[joedolson]

I can see that this ticket has gone down a totally different path from what I originally reported, which is in a context I wasn't even looking at when reporting. This really turns the problem into two separate issues, in my mind: fixing the reported problem (a function that doesn't work correctly) and fixing accessibility issues on the revisions page.

To move forward, I'm going to focus solely on fixing the problems in the function in this ticket, and open a new ticket for the purpose of addressing the revisions screen issues.

[joedolson]

Streamlined patch to handle functional differences

[joedolson]

Added patch covers the bare minimum to move this forward:

1) Fix structural problems in wp_text_diff and WP_Text_Diff_Renderer_Table
2) Call title_left and title_right arguments in revisions to add column headings.

Adding the column headings is helpful for understanding that screen, although this is a very minimal implementation. That still leaves some design requirements for this tickets, but can be dropped to be handled in #52303 if we can't determine that.

Making the structural changes eliminates the empty column space between the removals/additions columns. Personally, I don't see a problem with just not having that space; but this should also get a design look.

[joedolson]

Screenshot for 25473.4.diff

[mehulkaklotar]

Thank you @joedolson Finally

[estelaris]

the design team agreed with the UI changes after they have been approved by the accessibility team.

[joedolson]

In [50034]

Revisions: Generate correct number of columns in wp_text_diff.

The function wp_text_diff generated an invalid table structure if the $args parameter contained any values. This patch corrects the structure generated by wp_text_diff and related usages so that the column count matches the data generated. Additionally, this patch passes arguments to the Revisions screen so that the screen has column headings that reflect the content in each column. Improves the accessibility and usability of the Revisions table.

Props joedolson, mehulkaklotar, afercia, adamsilverstein, zodiac1978, jeremyfelt
Fixes #25473

Closing as ticket has been closed with changeset https://core.trac.wordpress.org/changeset/50034.

Closing as ticket has been closed with changeset https://core.trac.wordpress.org/changeset/50034.

Closing as ticket has been closed with changeset https://core.trac.wordpress.org/changeset/50034.