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

feat: Focus title if title is empty #9608

Merged
merged 5 commits into from Sep 7, 2018

Conversation

Projects
None yet
5 participants
@tofumatt
Member

tofumatt commented Sep 4, 2018

Description

Focuses the title when empty. Fixes #3828.

How has this been tested?

Tested locally in Firefox and Chrome for Mac; IE 11 and Edge in Windows.

Added an E2E test as well.

Screenshots

2018-09-04 14 27 40

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Sep 4, 2018

@@ -244,12 +244,12 @@ export const settings = {
} );
} );
// this checks for languages that do not typically have square brackets on their keyboards
// Check for languages that do not have square brackets on their keyboards.

This comment has been minimized.

@tofumatt

tofumatt Sep 4, 2018

Member

These changes are largely unrelated, but I forget why I found this file when working on this patch and just wanted to tidy things up. Sorry 😉

@tofumatt

tofumatt Sep 4, 2018

Member

These changes are largely unrelated, but I forget why I found this file when working on this patch and just wanted to tidy things up. Sorry 😉

Show outdated Hide outdated test/e2e/specs/new-post.test.js Outdated
Show outdated Hide outdated packages/editor/src/components/post-title/index.js Outdated

@tofumatt tofumatt requested a review from aduth Sep 4, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 4, 2018

Member

Changes made, good for another look!

Member

tofumatt commented Sep 4, 2018

Changes made, good for another look!

@tofumatt tofumatt self-assigned this Sep 5, 2018

@tofumatt tofumatt removed the request for review from aduth Sep 5, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

This will have another maybe "unwanted" side effect? If you switch to code mode and you switch back to editor without touching the title, it will be autofocused?

Shouldn't this be the same component, and thus not unmounted when switching modes?

Member

aduth commented Sep 5, 2018

This will have another maybe "unwanted" side effect? If you switch to code mode and you switch back to editor without touching the title, it will be autofocused?

Shouldn't this be the same component, and thus not unmounted when switching modes?

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 5, 2018

Member

It seems the layout of the editor is changed "above" the title component so things are re-rendered.

That's probably a legit bug but it might be beyond this one. 😄

Member

tofumatt commented Sep 5, 2018

It seems the layout of the editor is changed "above" the title component so things are re-rendered.

That's probably a legit bug but it might be beyond this one. 😄

@tofumatt tofumatt requested review from youknowriad and aduth Sep 6, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 6, 2018

Member

Okay! Improvements made; this now uses autofocus and behaves a lot better. All set for another review.

Member

tofumatt commented Sep 6, 2018

Okay! Improvements made; this now uses autofocus and behaves a lot better. All set for another review.

tofumatt added some commits Sep 4, 2018

@tofumatt tofumatt requested a review from aduth Sep 7, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 7, 2018

Member

Set for another review, sorry 'bout the delay! Thanks for the info on DOM attributes!

Member

tofumatt commented Sep 7, 2018

Set for another review, sorry 'bout the delay! Thanks for the info on DOM attributes!

@aduth

aduth approved these changes Sep 7, 2018 edited

LGTM 👍

I'm a little wary of the unintended remounts triggering focus grabs, but if anything it would be good for those to be surfaced (as disruptive as they might be) since the unintentionally surrounding this possibility should itself be considered a bug worth addressing.

A small thing, but it was a bit jarring for me to see the additional visual effect of the focused title (the border) when loading the editor. This is, of course, expected as a result of the title being auto-focused, though in the broader sense maybe we don't want to have such a heavy first impression. Worth iterating if it's sensed to be problematic.

image

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 7, 2018

Member

Fair enough, I could see the focus style being tweaked is the post is in the same state that enables autoFocus (eg isCleanNewPost).

Unrelated, but that's some Chrome theme you have there.

Member

tofumatt commented Sep 7, 2018

Fair enough, I could see the focus style being tweaked is the post is in the same state that enables autoFocus (eg isCleanNewPost).

Unrelated, but that's some Chrome theme you have there.

@tofumatt tofumatt merged commit b688d2e into master Sep 7, 2018

2 checks passed

codecov/project 49.07% remains the same compared to c1cdd95
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the 3828/feat-focus-empty-title branch Sep 7, 2018

@tofumatt tofumatt added this to the 3.9 milestone Sep 7, 2018

@designsimply

This comment has been minimized.

Show comment
Hide comment
@designsimply

designsimply Sep 14, 2018

Contributor

I tested and focus automatically goes to the title on new posts, but that does not happen for pre-existing published or draft posts with an empty title. Is this the intended behavior?

Tested with Firefox 62.0, Safari 11.1.2, and Chrome 69.0.3497.92 on macOS 10.13.6.

Contributor

designsimply commented Sep 14, 2018

I tested and focus automatically goes to the title on new posts, but that does not happen for pre-existing published or draft posts with an empty title. Is this the intended behavior?

Tested with Firefox 62.0, Safari 11.1.2, and Chrome 69.0.3497.92 on macOS 10.13.6.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 14, 2018

Member

That's the intended behaviour, yes. The idea is that any existing post that's been saved without a title was done so intentionally, so we don't focus the title in that case.

It would probably be nice if we eventually focused the last block on an existing page, but that's another issue 😄

For now nothing should be focused on an existing post.

Member

tofumatt commented Sep 14, 2018

That's the intended behaviour, yes. The idea is that any existing post that's been saved without a title was done so intentionally, so we don't focus the title in that case.

It would probably be nice if we eventually focused the last block on an existing page, but that's another issue 😄

For now nothing should be focused on an existing post.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

Since I just encountered it in an unrelated fix, what should be the expected behavior for a new post with a non-empty title, either via the_title filter or some initial edits (see Demo page).

#9921

Member

aduth commented Sep 14, 2018

Since I just encountered it in an unrelated fix, what should be the expected behavior for a new post with a non-empty title, either via the_title filter or some initial edits (see Demo page).

#9921

@designsimply

This comment has been minimized.

Show comment
Hide comment
@designsimply

designsimply Sep 24, 2018

Contributor

My take: since the demo post is a new post that has not yet been saved (bit of an edge case), so I think it's fine that auto-focus works for the title in that case.

Contributor

designsimply commented Sep 24, 2018

My take: since the demo post is a new post that has not yet been saved (bit of an edge case), so I think it's fine that auto-focus works for the title in that case.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Oct 14, 2018

Contributor

I'd like to propose to reconsider this for the demo post, if no objections? I'd tend to think the demo post should serve to familiarize with the whole UI. Skipping all the UI that's before the post title doesn't help screen reader users in discovering the features in the top bar. Actually, I'm not even sure if the demo post is meant to stay after merge, but what about checking explicitly if the post is empty? Something like autoFocus={ isCleanNewPost && ! title }. /Cc @tofumatt

Contributor

afercia commented Oct 14, 2018

I'd like to propose to reconsider this for the demo post, if no objections? I'd tend to think the demo post should serve to familiarize with the whole UI. Skipping all the UI that's before the post title doesn't help screen reader users in discovering the features in the top bar. Actually, I'm not even sure if the demo post is meant to stay after merge, but what about checking explicitly if the post is empty? Something like autoFocus={ isCleanNewPost && ! title }. /Cc @tofumatt

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 14, 2018

Member

Ah, that's a really good point @afercia. I'm not sure it's staying either but before I thought that edge case of focusing the title on the demo was fine because I wanted to err toward simple code. As it serves as an introduction to the editor, not focusing it seems good.

I mean, on the other hand, it means the intro to the editor won't focus the title like a new post would, but then a post like the demo page that had been saved wouldn't focus either. 🤷‍♂️

So yeah, I think that's a good point and we shouldn't focus it if a title is present. 👍

I've filed: #10601 to track it.

Member

tofumatt commented Oct 14, 2018

Ah, that's a really good point @afercia. I'm not sure it's staying either but before I thought that edge case of focusing the title on the demo was fine because I wanted to err toward simple code. As it serves as an introduction to the editor, not focusing it seems good.

I mean, on the other hand, it means the intro to the editor won't focus the title like a new post would, but then a post like the demo page that had been saved wouldn't focus either. 🤷‍♂️

So yeah, I think that's a good point and we shouldn't focus it if a title is present. 👍

I've filed: #10601 to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment