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

Post Preview Button: Use an <a> instead of a <button> #11187

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Oct 29, 2018

Fixes #9446.

Changes PostPreviewButton to use an <a> element when possible. Although we always intercept the onClick event, having the button rendered as an <a> with href and target attributes helps with accessibility, see #9446 (comment) for a great explanation on why.

As part of this, I've moved some code around and written some inline comments with an aim to make openPreviewWindow easier to comprehend.

Testing

I've updated the unit tests for PostButtonPreview.

The existing E2E test should serve as an acceptance test for these changes.

To test manually:

  1. Open a new post
  2. Type in some content
  3. Click Preview—it should generate a preview and display it in a new tab
  4. Go back to the post and publish it
  5. Make some more changes
  6. Click Preview—it should generate a preview and display it in the same tab

@noisysocks noisysocks added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 29, 2018
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Code looks good. Some really nice refactoring! The only usability thing I spotted is that the link can now not be opened using the spacebar, which feels like an inconsistency given that it's visually perceived as a button. I imagine users might have an expectation that it can be used in the same way as the 'Publish' button next to it.

@noisysocks what do you think?

@noisysocks
Copy link
Member Author

@talldan: That's a good point. I wonder if we should do this for all <Button>s with a href or just <PostPreviewButton>?

@noisysocks noisysocks added the Needs Accessibility Feedback Need input from accessibility label Oct 31, 2018
@talldan
Copy link
Contributor

talldan commented Oct 31, 2018

Would be good to get an idea on whether it's considered a blocker for this PR as well.

@aduth
Copy link
Member

aduth commented Oct 31, 2018

I don't know that the spacebar behavior should be a blocker here, as it applies to any <Button href>, including the preview button prior to when it was made a non-anchor.

I'm not sure what the correct answer is to the spacebar behavior of a link which takes the aesthetic of a button. I imagine the answer could include some commentary to the point of: maybe we should stop making links look like buttons. See also #7534.

Maybe worth creating an issue or consolidating to #7534 to be discussed separate to the effort here?

@noisysocks
Copy link
Member Author

Agree that it isn't a blocker. I dropped a note in #7534.

Would appreciate a quick check from @afercia or someone else on Accessibility before proceeding. Otherwise, I think this is good to get in.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I'll approve given the spacebar issue isn't considered a blocker.

@noisysocks noisysocks merged commit dd015c9 into master Nov 4, 2018
@noisysocks noisysocks deleted the fix/make-preview-button-a-link branch November 4, 2018 23:59
@noisysocks noisysocks added this to the 4.3 milestone Nov 4, 2018
@noisysocks
Copy link
Member Author

Thanks Dan. We can address any follow-up issues as seperate PRs.

@afercia
Copy link
Contributor

afercia commented Nov 5, 2018

Yep, the spacebar thing is a broader issue across the whole WordPress UI, definitely not a blocker for this PR. Thanks everyone for working on this.

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

Successfully merging this pull request may close these issues.

4 participants