Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#46041 closed defect (bug) (fixed)

Personal data tables: missing headings

Reported by: afercia's profile afercia Owned by: desrosj's profile desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-screenshots has-patch commit
Focuses: accessibility, administration Cc:

Description

In [34891] / #32147 and following changes, some visually hidden headings were added to all the list tables in the admin.

These headings expect their text to be passed via the current screen data and are important for screen reader users, as they allow to quickly jump to different sections in the page.

For example, in the Users page there are three visually hidden labels: see in the screenshot below, where I've made them temporarily visible.

In the new Export Personal Data and Erase Personal Data pages, the headings are missing and should be added for better accessibility.

Attachments (8)

table headings.png (33.1 KB) - added by afercia 6 years ago.
46041.diff (1.2 KB) - added by xkon 6 years ago.
Add screen reader text to privacy erasure/export tables
Screen Shot 2019-03-02 at 2.42.05 AM.png (94.6 KB) - added by garrett-eclipse 6 years ago.
Erase personal data screen reader text
Screen Shot 2019-03-02 at 2.45.01 AM.png (96.1 KB) - added by garrett-eclipse 6 years ago.
Export personal data screen reader text
46041.1.diff (1.2 KB) - added by xkon 6 years ago.
Title updates
46041.2.diff (1.0 KB) - added by desrosj 6 years ago.
Screen Shot 2019-03-07 at 10.51.24 PM.png (144.0 KB) - added by garrett-eclipse 6 years ago.
Retest of Data Erasure Request
Screen Shot 2019-03-07 at 10.48.11 PM.png (166.3 KB) - added by garrett-eclipse 6 years ago.
Restest of Data Export Request

Download all attachments as: .zip

Change History (21)

#1 @desrosj
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.2
  • Owner set to desrosj
  • Status changed from new to assigned

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


6 years ago

@xkon
6 years ago

Add screen reader text to privacy erasure/export tables

#3 @xkon
6 years ago

  • Keywords has-patch added; needs-patch removed

@desrosj , just a note for 46041.diff .

If we log a get_current_screen() in our _wp_personal_data_export_page() we can see an [id] as tools_page_export_personal_data. When logging inside the $requests_table->views(); though we'll get an [id] as export_personal_data.

To get the render_screen_reader_content working I've added the set_current_screen() as well but nor really sure if that would be the way to go.

@garrett-eclipse
6 years ago

Erase personal data screen reader text

@garrett-eclipse
6 years ago

Export personal data screen reader text

#4 @garrett-eclipse
6 years ago

  • Keywords needs-refresh added

Thanks @xkon the patch is working nicely and good call with the 'set_current_screen' I'd thought this might be blocked by #43895 but that's a nice workaround for now.

One minor note; for the heading_list can we add 'data' to them both so they match the naming applied to the rest of the page;
"Export personal list" => "Export personal data list"
"Erase personal list" => "Erase personal data list"

Only other thought/question I had was with 'Export Personal Data' and 'Erase Personal Data' being the page titles should the verbiage for screen readers use the capital case as well? In @afercia screenshot 'Filter users list' has users in lowercase but would capital case be more appropriate.

Attached screens of my tests.
Note: to produce the heading_pagination text you have to create enough requests for the pagination to be introduced.

@xkon
6 years ago

Title updates

#5 @xkon
6 years ago

  • Keywords needs-refresh removed

Woops! Thanks for spoting that @garrett-eclipse , patch updated in 46041.1.diff to add the missing "data".

#6 @garrett-eclipse
6 years ago

  • Keywords commit added

Thanks for addressing that @xkon. This looks and works nicely for me, marking for commit.

I added a note to #43895 about removing the set_current_screen calls when the files have been re-organized.

And the only other thought about page titles being capitalized in the screen reader text can be addressed in a unique ticket if @afercia feels there's any grounds there as it would affect most other instances of these three strings across other tables.

#7 @afercia
6 years ago

Thanks for working on this! Title casing isn't necessary :) Screen readers read out capital letters with a higher voice "pitch", which is annoying when not strictly necessary. This depends also if users are using commands to read content by sentence, or by word, or by character, so in this specific case it could be a minor concern. As general rule, avoiding title casing for screen reader text is recommended.

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


6 years ago

@desrosj
6 years ago

#9 @desrosj
6 years ago

  • Keywords commit removed

I don't like the idea of changing the current screen here as that would be a backward compatibility break. Plugins that are relying on tools_page_* would not know of this change.

I believe that 46041.2.diff solves the issue in a backward compatible way. It uses the screen object stored in the WP_List_Table instance to add the screen reader text (where the text is checked) to add the text instead of changing the global current page.

I imagine there are some weird behaviors being caused by these two not matching that have not been discovered yet. Unifying these can be tackled in #43895.

#10 @xkon
6 years ago

Thanks for the update on this @desrosj as I was a bit unsure as well as mentioned on doing a set_current_screen(). Patch applies & works as expected this way without the need of changing the screen name :) .

#11 @afercia
6 years ago

Thanks all for working on this! What's left here? Patch looks good for commit to me.

@garrett-eclipse
6 years ago

Retest of Data Erasure Request

@garrett-eclipse
6 years ago

Restest of Data Export Request

#12 @garrett-eclipse
6 years ago

  • Keywords commit added

Thanks @desrosj for the catch and refresh there, I've retested and everything seems honky dory. Marking for commit. Appreciate the report @afercia. And @xkon thanks for the tests and initial patch. Cheers

#13 @desrosj
6 years ago

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

In 44821:

Privacy: Add missing header text for screen readers to privacy list tables.

In [34891], WP_Screen was updated with methods to store, retrieve, and render screen reader text used by screens with WP_List_Table instances. When the export/erase personal data list tables were introduced in [42967], these headings were missing.

Fixes #46041.
Props afercia, xkon, garrett-eclipse, desrosj.

Note: See TracTickets for help on using tickets.