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

add/3016: Automatically select the upload image button on image blk focus #3142

Closed
wants to merge 1 commit into from

Conversation

BoardJames
Copy link

Description

Like editable automatically focus a relevant button when the block is focused. If this direction is acceptable a similar approach can be used on the other blocks mentioned in #3016 .

How Has This Been Tested?

Currently only manually tested in Firefox on linux.

Screenshots (jpeg or gifs if applicable):

TODO

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@BoardJames
Copy link
Author

@jasmussen is this change what you were after in #3016 ?

@codecov
Copy link

codecov bot commented Oct 25, 2017

Codecov Report

Merging #3142 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3142      +/-   ##
==========================================
- Coverage   31.67%   31.55%   -0.12%     
==========================================
  Files         217      217              
  Lines        6261     6284      +23     
  Branches     1112     1120       +8     
==========================================
  Hits         1983     1983              
- Misses       3598     3613      +15     
- Partials      680      688       +8
Impacted Files Coverage Δ
blocks/library/image/block.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9b429...f1220e0. Read the comment docs.

@jasmussen
Copy link
Contributor

I like this behavior a lot.

Is it kosher, @afercia?

@afercia
Copy link
Contributor

afercia commented Oct 26, 2017

Like editable automatically focus a relevant button when the block is focused.

Not clear to me, I don't see a button getting focus when a block is focused. I'm probably missing something 🙂

Is it kosher

Should be tested a bit with other combos; quickly testing just in Safari+VoiceOver the block type information gets announced in both cases, and this is important to give users a clue where they've just landed:

before:

screen shot 2017-10-26 at 12 31 59

after:

screen shot 2017-10-26 at 12 34 38

However, I have a couple usability concerns:

1
initial focus should be considered holistically: right now, there's no consistency across blocks and depending on the block, the behavior is often different. This should be standardized in some way. A couple options:

  • set initial focus on the place that makes the most sense for that type of block; this may be different for each block and end up in a not so easily predictable behavior: users would often be uncertain about where focus will land
  • always set initial focus on the same place for all the block types, e.g. a focusable container: this would guarantee a predictable behavior but it would require additional key presses to actually operate on the block

Overall, I think setting initial focus is often a developers assumption based on just one specific workflow. But this is just my personal opinion 🙂

2
In the specific case of the image block, there's no primary action: the two buttons represent two actions of the same importance: do this or do that. Instead, usually the initial focus should be set on a control representing the primary action, if any. So I'm not sure setting focus on the "Upload" button is 100% correct.

Aside: the text Drag image here or insert from media library doesn't mention all the available actions in this block. There's also "Upload".

@jasmussen
Copy link
Contributor

You make some good points Andrea. Alternately, the placeholder block needs a big focus style, which it doesn't appear to have now.

@BoardJames
Copy link
Author

Not clear to me, I don't see a button getting focus when a block is focused.

The button will be focused if the focus prop is set on the block and nothing within the block is already the active element (as per document.activeElement ). So typically the block only gets focused like this when it is newly created.

Technically I am not specifically choosing the button to be focused but just picking the first tabbable within the block.

@BoardJames
Copy link
Author

BoardJames commented Oct 27, 2017

@jasmussen I'm not clear on what I should do next. Should I?...

  • Leave as is
  • Focus the other button
  • Abandon the change
  • Some other option?

@afercia Does your advice relate to the other blocks that are mentioned in #3016 - for example I also made a similar change for the code block on the branch add/3016-auto-focus-code-block in that case there is nothing else to focus but the codeblock so would that one be ok?

@jasmussen
Copy link
Contributor

I'm not clear on what I should do next.

I think for now, this exploration should probably be left as is. No reason to close the PR, but let's let it sit here as we explore focus improvements first.

The key use case I was hoping to address with this change was the following text flow:

textflow

That is — when using the slash command to insert, focus was already on the buttons, because that felt natural. However Andreas points about being able to also focus the blocks make a lot of sense. So I'm going to explore separately a tab focus style for blocks.

@jasmussen
Copy link
Contributor

Chatted with Morgan in Slack about steps to reproduce the edgecase, here are steps and a GIF:

  • You'll need a paragraph of three or more lines and a block beneath it
  • Position your cursor on the last line of the first block and press Shift up twice
  • Then press shift+down
  • Watch how it expands the selection

GIF:

selection bug

It will only happen when the selection started on the last line, because when pressing down, it checks the end of your selection.

From Morgan on whether this is fixable:

You just need to use an algorithm to get the direction, and then make selection direction and not the direction of the arrow key determine whether you care about the start or the end of the selection

I feel like it's worth getting this branch into master before it grows more. Then we can track and fix this issue separately. Thoughts, @mtias, @aduth ?

@afercia
Copy link
Contributor

afercia commented Oct 27, 2017

@afercia Does your advice relate to the other blocks that are mentioned in #3016

Well not only those. For example, what about initial focus on blocks like pullquote, shortcode, video, audio...

My point was more about the fact there's a great variety of blocks. They often have different controls (editable, other fields, buttons). Custom blocks added by plugins may have a completely different UI. What would be the most predictable initial focus mechanism for users?

This should probably be standardized in a way both built-in and custom blocks have the same behavior.

One more important consideration is that custom block authors shouldn't worry about focus management. Gutenberg should handle focus for them.

@ephox-mogran
Copy link
Contributor

ephox-mogran commented Oct 29, 2017

@jasmussen I think your comment above (#3142 (comment)) is in the wrong issue :) Either way, it was merged as part of #3038

@BoardJames
Copy link
Author

Ok, I will shelve this issue (and the related PRs that I was working on) until focus and multi-block selection is finalized.

@danielbachhuber
Copy link
Member

@jasmussen @afercia Is this still relevant / actionable?

@danielbachhuber danielbachhuber added the [Status] Needs More Info Follow-up required in order to be actionable. label May 22, 2018
@jasmussen
Copy link
Contributor

No, I think it's superceded by some work @aduth is looking into. Thanks.

@danielbachhuber danielbachhuber deleted the add/3016-auto-focus-image-block branch May 22, 2018 16:56
@aduth
Copy link
Member

aduth commented May 23, 2018

Related issue: #6687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs More Info Follow-up required in order to be actionable.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants