Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Library: Button: Remove title attribute handling #19735

Merged
merged 2 commits into from
Feb 3, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 17, 2020

Previously: #17352, #19462 (comment), #19490 (comment), #616

This pull request seeks to remove UI controls which assign a title attribute for a block.

This is based on advisory resources and related core efforts which recommend that title attributes be avoided for links:

The proposed changes do not currently remove the attribute from the block definition, nor do they prevent the attribute from being applied when rendering the generating the block's saved markup. This is to avoid invalidations of existing blocks. We could potentially choose to implement a deprecation to remove the attribute altogether, but the implementation as proposed seemed simplest in that it merely prevents the addition of new titles.

Current Status: I'm considering this as blocked by the issue described in #19651 (comment), where an empty title would introduce a confusing user experience for editing links.

Testing Instructions:

Verify that when selecting a link for a Button block, the resulting post markup does not contain a title attribute:

  1. Navigate to Posts > Add New
  2. Insert a Buttons block
  3. Assign a link to the button (either to an external URL, or a search result for a post within the site)
  4. Change to Code Editor using the editor's top-right "More Options" menu
  5. Verify that the resulting markup does not contain a title attribute

Follow-up Tasks: If these changes are considered acceptable, a similar effort should be undertaken to remove the assignment of title attribute for the navigation link block:

if ( isset( $block['attrs']['title'] ) ) {
$html .= ' title="' . esc_attr( $block['attrs']['title'] ) . '"';
}

The "Image" block also includes title assignment, but my understanding is that it is not quite as problematic for use in images (see also #11054).

@aduth aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] Blocked Used to indicate that a current effort isn't able to move forward Needs Accessibility Feedback Need input from accessibility [Block] Buttons Affects the Buttons Block labels Jan 17, 2020
@MarcoZehe
Copy link
Contributor

I'm all for this and the follow-ups. The less title attributes, the better for keyboard, touch, and screen readers.

Out of curiosity: I saw that you remove a title attribute on a div element here. Did that actually ever do anything useful even for sighted people? :-)

@aduth
Copy link
Member Author

aduth commented Jan 20, 2020

Out of curiosity: I saw that you remove a title attribute on a div element here. Did that actually ever do anything useful even for sighted people? :-)

Practically speaking, it was applying the title attribute to one of the button's wrapping elements in the editor. It does seem to have at least have had the impact of showing a visual tooltip when hovering over the button with the cursor. It's debatable that this was actually very useful, outside of replicating how the button would behave when shown on the front of the site.

@aduth
Copy link
Member Author

aduth commented Jan 20, 2020

The UX for an absent link title has been improved in #19739, so I'll remove the "[Status] Blocked" label.

Thanks @MarcoZehe for your comments at #19735 (comment) . I'll remove "Needs Accessibility Feedback" label accordingly.

I might like to invite some feedback from one or more of @mtias @getdave or @jorgefilipecosta in case there might be some justification to consider that we've missed for keeping these attributes.

@aduth aduth removed Needs Accessibility Feedback Need input from accessibility [Status] Blocked Used to indicate that a current effort isn't able to move forward labels Jan 20, 2020
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided that we are ok with removing title attribute from links on the "a" element on the frontend it looks good to me 👍
I think it may make sense to add a comment in save.js above the title attribute saying that right now the attribute is never set?

@aduth
Copy link
Member Author

aduth commented Jan 30, 2020

I think it may make sense to add a comment in save.js above the title attribute saying that right now the attribute is never set?

That's a good thought. I added a comment in d32c29d.

@aduth aduth force-pushed the update/button-remove-title branch from e4b8957 to 670ed98 Compare February 3, 2020 16:43
@aduth aduth requested review from nerrad and ntwb as code owners February 3, 2020 17:04
@aduth aduth merged commit f1e45f4 into master Feb 3, 2020
@aduth aduth deleted the update/button-remove-title branch February 3, 2020 23:24
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 3, 2020
@aduth
Copy link
Member Author

aduth commented Feb 3, 2020

As a follow-up to this, I created #19990 to remove the title HTML attribute from the Navigation Link block links as well.

@aduth
Copy link
Member Author

aduth commented Feb 12, 2020

See related: #20198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants