WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 13 days ago

Last modified 7 days ago

#43929 closed defect (bug) (fixed)

Privacy pages: buttons should be buttons and other coding standards

Reported by: afercia Owned by:
Milestone: 4.9.6 Priority: normal
Severity: normal Version: trunk
Component: Privacy Keywords: gdpr has-patch needs-testing
Focuses: coding-standards Cc:

Description

Starting with [42986] and following commits, a few buttons were added in the Privacy pages, to allow users export and remove their personal data. These buttons are actually links with a href="#" attribute (9 occurrences).

As per the accessibility standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/#semantics-for-controls) these links should be real buttons: <button type="button" ...

Worth noting the type="button" attribute allows to not use event.preventDefault() as there's no default action to prevent.

===

CSS selectors:

As per the CSS coding standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors) words should be separated with hyphens, however many selectors in these pages use underscores:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors

=== JavaSCript variable declarations with var

As per the JavaScript coding standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#declaring-variables-with-var), multiple var var var declarations should be avoided:

Each function should begin with a single comma-delimited var statement that declares any local variables necessary.

I know this is going to change, but the current standards are very clear. I'd say either update the coding standards, or meet them :)

Attachments (6)

43929.diff (16.4 KB) - added by xkon 3 weeks ago.
43929.2.diff (16.3 KB) - added by desrosj 2 weeks ago.
Refreshes previous patch.
43929.3.diff (16.5 KB) - added by desrosj 2 weeks ago.
43929.4.diff (16.4 KB) - added by desrosj 2 weeks ago.
43929.5.diff (16.7 KB) - added by desrosj 2 weeks ago.
43929.6.diff (19.0 KB) - added by xkon 2 weeks ago.

Download all attachments as: .zip

Change History (25)

#1 @audrasjb
3 weeks ago

  • Keywords gdpr added

#2 @netweb
3 weeks ago

  • Keywords needs-patch added

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


3 weeks ago

@xkon
3 weeks ago

#4 @xkon
3 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

43929.diff

Thanks @afercia for taking time to check things.

  • converts <a> to <buttons> where needed in user.php
  • converts all css classes with - instead of _ in user.php
  • groups vars in xfn.js
  • fixes various coding standards in user.php / xfn.js

I tested everything ( using trunk ) but please do thorough tests as all action handlers etc have been touched :]

Note: I didn't mess with non-privacy code so there are still a couple of CS notices in those files.

cc @allendav ( pinging since you know these actions better than anyone :D )

@desrosj
2 weeks ago

Refreshes previous patch.

@desrosj
2 weeks ago

#5 @desrosj
2 weeks ago

Did a bunch of testing on this. Everything seems to be working for me. 43929.3.diff converts a few more <a href="#" occurrences to buttons.

#6 @audrasjb
2 weeks ago

  • Keywords needs-testing removed

Hey, I was typing something about type="button" missing parameter, but it seems fine with 43929.3.diff now. I tested the patch on my side and I think it is okay to land in 4.9.6. Thanks.

#7 @audrasjb
2 weeks ago

…but I guess you don't need no event.preventDefault(); anymore since we are using <button type="button"> HTML elements.

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


2 weeks ago

#9 @allendav
2 weeks ago

  • Keywords needs-refresh added

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


2 weeks ago

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


2 weeks ago

@desrosj
2 weeks ago

#12 @desrosj
2 weeks ago

  • Keywords needs-testing added; needs-refresh removed

43929.4.diff is a refresh. Some of the action label text had changed (r43175), so please double check that for me.

Also in the refresh:

  • Remove preventDefault()
  • Ensure all <buttons> have type="button"
  • Change translation inline comments to use /* translators: a note */ instead of // translators: a note */

#13 @allendav
2 weeks ago

Applies cleanly, tests fairly well. Two things:

"Force Remove Personal Data" should probably be "Force Erase Personal Data" for consistency.

Also, I did notice that our "Remove request" links are still anchors. (Remove is appropriate for requests, just not for personal data - that should be erase.)

@desrosj
2 weeks ago

#14 @desrosj
2 weeks ago

Thanks, @allendav. I fixed that missed text. I also added class="button" to the remove request links, but left them as links.

Looking at the a11y handbook, it seems like these links are recommended to be <a> tags.

If there is a valid target link for the control, either an in-page reference or a link, then the control should use an <a href="">

@afercia could confirm. But I think this is ready to go.

@xkon
2 weeks ago

#15 @xkon
2 weeks ago

Some extra minor CS fixes in 43929.6.diff . Other than that it looks good to me and applies & works without issues @desrosj .

wp-admin\js\xfn.js

@ 218 spaces to tabs 
@ 222 spaces to tabs
@ 227 spaces to tabs
@ 229 spaces to tabs
@ 234 new line above comment
@ 264 convert `} )( jQuery );` to `} ( jQuery ) );`
wp-admin\includes\user.php

@ 666 remove empty line
@ 1115 space before `: '';`
@ 1218 space before `: 0;`
@ 1480 `,` after array
wp-includes\user.php
@ 3081 align `=`
@ 3114 space before `: 0;`
Last edited 2 weeks ago by xkon (previous) (diff)

#16 @azaozz
13 days ago

In 43212:

Privacy: cleanup of the "Export Personal Data" and "Erase Personal Data" screens.

Props desrosj, xkon.
See #43929.

#17 @azaozz
13 days ago

In 43213:

Privacy: cleanup of the "Export Personal Data" and "Erase Personal Data" screens.

Props desrosj, xkon.
Merges [43212] to the 4.9 branch.
See #43929.

#18 @azaozz
13 days ago

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

Meant to fix this with [43213].

#19 @desrosj
7 days ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.