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

Input Interaction: Arrow keys become stuck in paragraph with background color #14702

Closed
aduth opened this issue Mar 29, 2019 · 15 comments · Fixed by #14804
Closed

Input Interaction: Arrow keys become stuck in paragraph with background color #14702

aduth opened this issue Mar 29, 2019 · 15 comments · Fixed by #14804
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended

Comments

@aduth
Copy link
Member

aduth commented Mar 29, 2019

Describe the bug

When a background color is assigned to a paragraph, it is not possible to use ArrowUp or ArrowDown to navigate out of the block.

To Reproduce

  1. Navigate to Posts > Add New
  2. Insert two paragraph blocks with some content
  3. Apply a background color to the first paragraph
  4. Select the first paragraph block
  5. Press ArrowUp or ArrowDown repeatedly
  6. Note that focus never leaves the paragraph

Expected behavior

ArrowUp should move focus to the title.
ArrowDown should move focus to the next block.

ArrowLeft and ArrowRight appear to work as expected when at the respective extents of the block content.

This behaves as expected if the paragraph block does not have a background color assigned.

Desktop (please complete the following information):

  • OS: macOS Mojave Version 10.14.3 (18D109)
  • Browser: Chrome Version 73.0.3683.86 (Official Build) (64-bit)

Additional context

Likely an issue in one of these two functions:

export function isVerticalEdge( container, isReverse ) {

cc @ellatrix

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Mar 29, 2019
@ellatrix
Copy link
Member

@aduth Is this something that has recently come up (did it work before)?

@aduth
Copy link
Member Author

aduth commented Mar 29, 2019

It's an issue in WordPress 5.1.1. I haven't checked earlier. Not as recent as some of your latest refactors, I would guess?

@ellatrix
Copy link
Member

ellatrix commented Apr 2, 2019

@aduth I cannot reproduce this issue in the latest master (also using Chrome). Can you reproduce it there as well?

@aduth
Copy link
Member Author

aduth commented Apr 2, 2019

I can still reproduce this in the current master branch, yes.

@ellatrix
Copy link
Member

ellatrix commented Apr 2, 2019

@aduth Hm, I followed your steps once again, but I still cannot reproduce. Here's a GIF:

background

@aduth
Copy link
Member Author

aduth commented Apr 3, 2019

🤷‍♂️ Honestly not sure. I've reproduced in multiple browsers (Chrome, Firefox Developer Edition) in multiple sites (local and remote).

Maybe worth a third opinion to break the tie on whether this is only affecting me? Someone from @WordPress/gutenberg-core perhaps want to try to see if they can reproduce?

@adamsilverstein
Copy link
Member

adamsilverstein commented Apr 3, 2019

I can confirm this is a bug, testing on wp trunk and gb master. With a background color the up and down arrows gets 'stuck' in the block.

Screencast:

@kjellr
Copy link
Contributor

kjellr commented Apr 3, 2019

I can reproduce on master too. The arrow keys will move the cursor to the beginning or end of the text, but they won't let it leave the paragraph.

updown

MacOS 10.14.3
Safari 12.0.3, as well as Chrome 73.0.3683.86

@ellatrix
Copy link
Member

ellatrix commented Apr 3, 2019

Very strange... I still cannot reproduce.

What theme do you all have enabled? I have Twenty Nineteen enabled. Perhaps it's some CSS issue.

@kjellr
Copy link
Contributor

kjellr commented Apr 3, 2019

What theme do you all have enabled? I have Twenty Nineteen enabled. Perhaps it's some CSS issue.

Ah yes. Oddly enough, this seems to work fine in Twenty Nineteen (and Twenty Fifteen for that matter).

I can reproduce in all other themes I've tried though: The Gutenberg Starter theme, Twenty Seventeen, Twenty Sixteen.

@ellatrix
Copy link
Member

ellatrix commented Apr 3, 2019

Ah, the difference is that the paragraphs have a margin in those themes, a padding in others. I'll fix it to account for that.

@ellatrix
Copy link
Member

ellatrix commented Apr 3, 2019

Actually, that's not it. It's the styled the same way. Maybe the problem is the buffer calculation, not sure.

@aduth
Copy link
Member Author

aduth commented Apr 3, 2019

Maybe the problem is the buffer calculation, not sure.

From my uninformed perspective, that sounds quite likely. Or, at least last time I had looked at this code, it seemed there were unaccounted factors like padding, line-height, font-size, etc, remarked as well in #12322 (comment) :

ideally [...] the buffer doesn't exist because we improve the implementation where the buffer was introduced as a band-aid to account for what may otherwise be explained by factors like padding or line height

If it's not possible to have a perfect calculation of that gap, maybe it's enough as an improvement to at least incorporate (add) container padding, line-height, font-size to the existing buffer?

@afercia
Copy link
Contributor

afercia commented Apr 10, 2019

I can reproduce in Twenty Seventeen. Not sure it can help, but when I uncheck these paddings in the Chrome dev tools, then arrows navigation works as expected:

.wp-block-paragraph.has-background {
    padding: 20px 30px;
}

p.has-background {
    padding: 20px 30px;
}

@ellatrix
Copy link
Member

#14804 fixes this issue, might be worth adding to the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants