Make WordPress Core

Opened 10 years ago

Closed 3 years ago

Last modified 3 years ago

#25473 closed defect (bug) (fixed)

wp_text_diff creates wrong number of columns if title arguments are set

Reported by: joedolson's profile joedolson Owned by: joedolson's profile joedolson
Milestone: 5.7 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 (14)

25473.wp-text-diff.patch (758 bytes) - added by joedolson 10 years ago.
Patch to wp_text_diff
markup-table.diff (711 bytes) - added by mehulkaklotar 9 years ago.
th with colspan 2
Screenshot from 2015-07-31 12:22:33.png (14.1 KB) - added by mehulkaklotar 9 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 years 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 5 years ago.
25473.diff (13.5 KB) - added by afercia 5 years ago.
split view.png (31.4 KB) - added by afercia 5 years ago.
wp_text_diff() split view
inline view.png (30.5 KB) - added by afercia 5 years ago.
wp_text_diff() inline view
Revisions page.png (239.6 KB) - added by afercia 5 years ago.
Revisions page with patch applied
25473.2.diff (14.2 KB) - added by adamsilverstein 5 years ago.
Image 2019-08-12 at 6.13.20 PM.jpg (375.6 KB) - added by adamsilverstein 5 years ago.
25473.3.diff (14.1 KB) - added by adamsilverstein 3 years ago.
25473.4.diff (4.0 KB) - added by joedolson 3 years ago.
Streamlined patch to handle functional differences
revisions-25473.4.diff.png (38.5 KB) - added by joedolson 3 years ago.
Screenshot for 25473.4.diff

Download all attachments as: .zip

Change History (87)

@joedolson
10 years ago

Patch to wp_text_diff

#1 @c4xpl0siv3
10 years ago

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

Tested and fine

#2 @c4xpl0siv3
10 years ago

  • Keywords 2nd-opinion close added

#3 @joedolson
10 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
10 years ago

  • Milestone Awaiting Review deleted

Clearing out the milestones for closed tickets on Awaiting Review

#5 @zodiac1978
9 years ago

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

I can reproduce this bug. Re-opening.

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

  • Milestone set to Awaiting Review

#8 @mehulkaklotar
9 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
9 years ago

th with colspan 2

@mehulkaklotar
9 years ago

td and th creates problem in markup and display

#9 @mehulkaklotar
9 years ago

  • Severity changed from normal to major

#10 @mehulkaklotar
8 years ago

  • Keywords ui-feedback added

#11 @johnbillion
8 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
8 years ago

#36127 was marked as a duplicate.

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

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

#18 @afercia
5 years ago

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

@afercia
5 years ago

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

wp_text_diff() split view

@afercia
5 years ago

wp_text_diff() inline view

@afercia
5 years ago

Revisions page with patch applied

#20 @adamsilverstein
5 years ago

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

#21 @adamsilverstein
5 years ago

25473.2.diff updates language and formatting slightly.

#22 @afercia
5 years 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
5 years 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
5 years 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
5 years 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.


5 years ago

#27 @afercia
5 years ago

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

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

  • Keywords needs-design-feedback needs-design added

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


4 years ago

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


4 years ago

#34 @karmatosed
4 years 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
4 years 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.

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


4 years ago

#37 @melchoyce
4 years ago

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.

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


4 years ago

#39 @audrasjb
4 years ago

  • Milestone changed from 5.4 to 5.5

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

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


4 years ago

This ticket was mentioned in PR #226 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#41

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

-->

Trac ticket:

#42 @melchoyce
4 years ago

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

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


4 years ago

#44 @estelaris
4 years ago

  • Keywords needs-screenshots added; has-screenshots removed

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


4 years ago

#46 @adamsilverstein
4 years ago

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

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


4 years ago

#48 @davidbaumwald
4 years ago

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

#49 @adamsilverstein
4 years ago

  • Milestone changed from 5.5 to Future Release

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.

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


4 years ago

This ticket was mentioned in PR #566 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#51

Trac ticket:

#52 @adamsilverstein
3 years ago

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.

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


3 years ago

#54 @paaljoachim
3 years ago

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

#55 @karmatosed
3 years ago

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

#56 @adamsilverstein
3 years ago

  • Milestone changed from Future Release to 5.7

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.

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


3 years ago

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


3 years ago

#59 @ryokuhi
3 years ago

  • Owner changed from adamsilverstein to joedolson

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

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


3 years ago

#61 @estelaris
3 years ago

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

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


3 years ago

#63 @joedolson
3 years ago

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
3 years ago

Streamlined patch to handle functional differences

#64 @joedolson
3 years ago

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
3 years ago

Screenshot for 25473.4.diff

#65 @mehulkaklotar
3 years ago

Thank you @joedolson Finally

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


3 years ago

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


3 years ago

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


3 years ago

#69 @estelaris
3 years ago

  • Keywords has-screenshots added; needs-design-feedback needs-design needs-screenshots removed

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

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


3 years ago

#71 @joedolson
3 years ago

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

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

hellofromtonya commented on PR #566:


3 years ago
#72

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

hellofromtonya commented on PR #226:


3 years ago
#73

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

hellofromtonya commented on PR #80:


3 years ago
#74

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

Note: See TracTickets for help on using tickets.