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 publish panel: focus the post link #11489

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 5, 2018

Addresses part of #4187 (comment)

At the moment, when the post-publish panel is opened the focus is at the non-interactive "Published" div element in the header. We want to have the focus in a meaningful element that is also tabbable. This PR makes the post link element to grab the focus once the post-publish panel opens.

@oandregal oandregal self-assigned this Nov 5, 2018
@oandregal oandregal added this to the 4.3 milestone Nov 5, 2018
@oandregal oandregal added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress labels Nov 5, 2018
@oandregal oandregal requested a review from tofumatt November 5, 2018 12:07
@oandregal oandregal removed the [Status] In Progress Tracking issues with work in progress label Nov 5, 2018
@oandregal oandregal changed the title Focus the post link on the PostPublishPanelPostPublish Post publish panel: focus the post link Nov 5, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This is an improvement but it exposes a pretty serious usability/accessibility flaw, related to #9396: if you enable the "private" option you get an alert and it's difficult (though, as illustrated, not impossible) to logically tab to the "password-protected" option. Here's a recording:

2018-11-05 22 13 10

That said this is an improvement.

I also noticed some weird re-renders where the entire panel renders again and focus changes but I think that's also unrelated.


OMG okay I wrote all of that and realised that's the pre-publish panel. 😓 I get them confused a lot. I guess it still applies but I should file those issues separately. 🤷‍♂️


This is great; I see the improvement and it's solid. Sorry 😅

const propsForPanel = omit( additionalProps, [ 'hasPublishAction', 'isDirty' ] );
const isPublishedOrScheduled = isPublished || ( isScheduled && isBeingScheduled );
const isPrePublish = ! isPublishedOrScheduled && ! isSaving;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this isSaving check causes re-renders if an autosave fires after the panel is open? I saw some weird re-renders on slower connections (I'm working from the pub right now) and I wonder if a late auto-save after the publish panel is opened causes this to close the panel and then open it again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about how the autosave mechanism works, but I've just checked the editor store code and isSaving (that holds state.saving.request) only changes when the content has been edited. So, the spinner will be shown instead of the pre-publish panel when:

  • the post contents are changed while the panel is open
  • the panel was opened before the auto-save kicked in

Note that I couldn't reproduce this with the post-publish panel open.

For reference, before #11013 this wasn't probably happening because we used some internal state (submitted) to keep track of this, which had introduced a bug in scheduling posts.

Happy to revisit this in a separate PR if needed. Just assign it to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to mention: I've noticed that focus is retained at the component that had it after spinner is rendered.

@oandregal oandregal merged commit e6fb93e into master Nov 6, 2018
@oandregal oandregal deleted the update/post-publish-panel-focus branch November 6, 2018 12:10
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).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants