WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 19 months ago

Last modified 19 months 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: 5.1
Component: Privacy Keywords: gdpr has-patch needs-testing
Focuses: coding-standards Cc:
PR Number:

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 20 months ago.
43929.2.diff (16.3 KB) - added by desrosj 20 months ago.
Refreshes previous patch.
43929.3.diff (16.5 KB) - added by desrosj 20 months ago.
43929.4.diff (16.4 KB) - added by desrosj 19 months ago.
43929.5.diff (16.7 KB) - added by desrosj 19 months ago.
43929.6.diff (19.0 KB) - added by xkon 19 months ago.

Download all attachments as: .zip

Change History (25)

#1 @audrasjb
20 months ago

  • Keywords gdpr added

#2 @netweb
20 months ago

  • Keywords needs-patch added

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


20 months ago

@xkon
20 months ago

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

Refreshes previous patch.

@desrosj
20 months ago

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


20 months ago

#9 @allendav
20 months ago

  • Keywords needs-refresh added

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


19 months ago

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


19 months ago

@desrosj
19 months ago

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

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

#15 @xkon
19 months 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;`
Version 0, edited 19 months ago by xkon (next)

#16 @azaozz
19 months ago

In 43212:

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

Props desrosj, xkon.
See #43929.

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

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

Meant to fix this with [43213].

#19 @desrosj
19 months ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.