Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#43929 closed defect (bug) (fixed)

Privacy pages: buttons should be buttons and other coding standards

Reported by: afercia's profile afercia Owned by:
Milestone: 4.9.6 Priority: normal
Severity: normal Version: 5.1
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 7 years ago.
43929.2.diff (16.3 KB) - added by desrosj 7 years ago.
Refreshes previous patch.
43929.3.diff (16.5 KB) - added by desrosj 7 years ago.
43929.4.diff (16.4 KB) - added by desrosj 7 years ago.
43929.5.diff (16.7 KB) - added by desrosj 7 years ago.
43929.6.diff (19.0 KB) - added by xkon 7 years ago.

Download all attachments as: .zip

Change History (25)

#1 @audrasjb
7 years ago

  • Keywords gdpr added

#2 @netweb
7 years ago

  • Keywords needs-patch added

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


7 years ago

@xkon
7 years ago

#4 @xkon
7 years 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
7 years ago

Refreshes previous patch.

@desrosj
7 years ago

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


7 years ago

#9 @allendav
7 years ago

  • Keywords needs-refresh added

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


7 years ago

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


7 years ago

@desrosj
7 years ago

#12 @desrosj
7 years 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
7 years 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
7 years ago

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

#15 @xkon
7 years 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 7 years ago by xkon (previous) (diff)

#16 @azaozz
7 years ago

In 43212:

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

Props desrosj, xkon.
See #43929.

#17 @azaozz
7 years 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
7 years ago

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

Meant to fix this with [43213].

#19 @desrosj
7 years ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.