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

Fix regression with textbox spacing + a focus issue #9186

Merged
merged 2 commits into from Aug 22, 2018

Conversation

Projects
None yet
5 participants
@jasmussen
Contributor

jasmussen commented Aug 21, 2018

This PR fixes two things.

  1. It fixes a small regression where we removed the 1px margin on input fields inherited by WordPress. But we did this with too much specificity so it regressed other input fields as well. To test this works now publish a post and verify that the URL copy input field looks right.

After:

screen shot 2018-08-21 at 09 22 07

  1. It removes a focus style we used to have a while back. Back then if you were in isEditing mode, where all UI fade out, arrow-keying to a placeholder or image block would not show the block UI. This focus style then at least showed it as highlighted black. Since those are now shown as "selected" effectively exiting isEditing mode, this old focus fix is no longer needed. Not only that but this focus style has increasingly as part of other changes become a focus style you accidentally invoke on the text appender.

Before:

screen shot 2018-08-21 at 09 10 10

I do still miss the days when you could use the slash command to insert an image placeholder, then press tab tab to go to the "open media library" dialog. Right now you have to tab through all the side UI and block UI to get there. But this is something to think about separately.

Fix regression with textbox spacing + a focus issue
This PR fixes two things.

1. It fixes a small regression where we removed the 1px margin on input fields inherited by WordPress. But we did this with too much specificity so it regressed other input fields as well. To test this works now publish a post and verify that the URL copy input field looks right.

2. It removes a focus style we used to have a while back. Back then if you were in `isEditing` mode, where all UI fade out, arrow-keying to a placeholder or image block would not show the block UI. This focus style then at least showed it as highlighted black. Since those are now shown as "selected" effectively exiting isEditing mode, this old focus fix is no longer needed. Not only that but this focus style has increasingly as part of other changes become a focus style you accidentally invoke on the text appender.

I do still miss the days when you could use the slash command to insert an image placeholder, then press tab tab to go to the "open media library" dialog. Right now you have to tab through all the side UI and block UI to get there. But this is something to think about separately.

@jasmussen jasmussen added this to the 3.7 milestone Aug 21, 2018

@jasmussen jasmussen self-assigned this Aug 21, 2018

@jasmussen jasmussen requested review from aduth and WordPress/gutenberg-core Aug 21, 2018

@tofumatt

Code seems fine and it works for me, just curious if this has accessibility ramifications?

@@ -87,7 +87,7 @@
}
.components-button {
text-align: center;
justify-content: center;

This comment has been minimized.

@tofumatt

tofumatt Aug 21, 2018

Member

Odd, justify-content says it doesn't work in IE 11 at all and is buggy in Edge with Flexbox. https://caniuse.com/#search=justify-content

But it looks good to me in IE11:

screenshot 2018-08-21 09 21 13

@tofumatt

tofumatt Aug 21, 2018

Member

Odd, justify-content says it doesn't work in IE 11 at all and is buggy in Edge with Flexbox. https://caniuse.com/#search=justify-content

But it looks good to me in IE11:

screenshot 2018-08-21 09 21 13

This comment has been minimized.

@jasmussen

jasmussen Aug 21, 2018

Contributor

Text align did nothing, for whatever reason.

@jasmussen

jasmussen Aug 21, 2018

Contributor

Text align did nothing, for whatever reason.

@@ -115,19 +115,6 @@
}
}
// This is a focus style shown for blocks that need an indicator even when in an isEditing state
// like for example an image block that receives arrowkey focus.
.edit-post-visual-editor .editor-block-list__block:not(.is-selected) {

This comment has been minimized.

@tofumatt

tofumatt Aug 21, 2018

Member

Is this okay to remove for a11y? Why did they need that state?

@tofumatt

tofumatt Aug 21, 2018

Member

Is this okay to remove for a11y? Why did they need that state?

This comment has been minimized.

@jasmussen

jasmussen Aug 21, 2018

Contributor

It's a bit hard to explain, there are GIFs in the archives here. But here's a GIF of the behavior today:

focus

Back in the day, the "isEditing" mode that fades out UI as you're typing remained faded out even when you navigated into a non text block. This would then get a black outline as I just commented out.

@jasmussen

jasmussen Aug 21, 2018

Contributor

It's a bit hard to explain, there are GIFs in the archives here. But here's a GIF of the behavior today:

focus

Back in the day, the "isEditing" mode that fades out UI as you're typing remained faded out even when you navigated into a non text block. This would then get a black outline as I just commented out.

This comment has been minimized.

@tofumatt

tofumatt Aug 21, 2018

Member

Coolio, that makes sense. Good to 🚢 in my mind!

@tofumatt

tofumatt Aug 21, 2018

Member

Coolio, that makes sense. Good to 🚢 in my mind!

This comment has been minimized.

@youknowriad

youknowriad Aug 29, 2018

Contributor

Why are we losing the "isEditing" mode when we move to a non-textual block at the moment?

@youknowriad

youknowriad Aug 29, 2018

Contributor

Why are we losing the "isEditing" mode when we move to a non-textual block at the moment?

This comment has been minimized.

@jasmussen

jasmussen Aug 29, 2018

Contributor

I can't recall the details but I believe it was an accessibility thing about the toolbar. I think maybe @aduth worked on it but not sure.

@jasmussen

jasmussen Aug 29, 2018

Contributor

I can't recall the details but I believe it was an accessibility thing about the toolbar. I think maybe @aduth worked on it but not sure.

This comment has been minimized.

@aduth

aduth Aug 29, 2018

Member

Context: #5552

Notably, as otherwise:

" it would be impossible to use the image toolbar by keyboard alone "

@aduth

aduth Aug 29, 2018

Member

Context: #5552

Notably, as otherwise:

" it would be impossible to use the image toolbar by keyboard alone "

This comment has been minimized.

@aduth

aduth Aug 29, 2018

Member

The function comment for ObserveTyping's stopTypingOnNonTextField could probably be improved to make clearer its purpose.

@aduth

aduth Aug 29, 2018

Member

The function comment for ObserveTyping's stopTypingOnNonTextField could probably be improved to make clearer its purpose.

This comment has been minimized.

@jasmussen

jasmussen Aug 30, 2018

Contributor

I would also note that it could be revisited in the future, perhpas with some of the focus mode toolbar improvements. But I would also admit this to be low priority.

@jasmussen

jasmussen Aug 30, 2018

Contributor

I would also note that it could be revisited in the future, perhpas with some of the focus mode toolbar improvements. But I would also admit this to be low priority.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 21, 2018

Contributor

I'm mobile and 🏖 can't fully test the focus thing but I trust @jasmussen 🙂
Worth noting (last time I've checked) the URL input field was missing a label. It needs a label associated with for/id attributes, not sure if it should be addressed in this PR.

Contributor

afercia commented Aug 21, 2018

I'm mobile and 🏖 can't fully test the focus thing but I trust @jasmussen 🙂
Worth noting (last time I've checked) the URL input field was missing a label. It needs a label associated with for/id attributes, not sure if it should be addressed in this PR.

@aduth

Reviewing only the black focus border changes: In extended back-and-forth with @jasmussen , it seems this is a lingering fragment which is now better represented by the .is-selected styling which is applied to a block which becomes focused. The black border appears currently only because of some incidental behavior† for the empty default block not applying the .is-selected class. These styles should be fine to remove altogether.

const shouldAppearSelected = ! showSideInserter && isSelected && ! isTypingWithinBlock;

'is-selected': shouldAppearSelected,

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 22, 2018

Contributor

Worth noting (last time I've checked) the URL input field was missing a label. It needs a label associated with for/id attributes, not sure if it should be addressed in this PR.

I think that's separate, and worth ticketing if it isn't already. This PR only touches a margin regression, of which the "copy URL" field happened to be affected.

Contributor

jasmussen commented Aug 22, 2018

Worth noting (last time I've checked) the URL input field was missing a label. It needs a label associated with for/id attributes, not sure if it should be addressed in this PR.

I think that's separate, and worth ticketing if it isn't already. This PR only touches a margin regression, of which the "copy URL" field happened to be affected.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 22, 2018

Contributor

@aduth forgive me as I'm having a hard time parsing your comment. Did you mean to suggest I had to remove additional code? Or was your comment in approval that this PR is good to ship?

Contributor

jasmussen commented Aug 22, 2018

@aduth forgive me as I'm having a hard time parsing your comment. Did you mean to suggest I had to remove additional code? Or was your comment in approval that this PR is good to ship?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 22, 2018

Member

My comment had no objections. Consider it approval for the one half of the pull request I targeted 😉

Member

aduth commented Aug 22, 2018

My comment had no objections. Consider it approval for the one half of the pull request I targeted 😉

@jasmussen jasmussen merged commit ae3597f into master Aug 22, 2018

2 checks passed

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

@jasmussen jasmussen deleted the fix/input-spacing-and-focus branch Aug 22, 2018

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