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

Framework: Drop the focus/setFocus props from block edit functions #4872

Merged
merged 11 commits into from
Feb 7, 2018

Conversation

youknowriad
Copy link
Contributor

The idea of this PR is simple: We only have to worry about the position of the cursor when selecting a new block, everything else should be left as uncontrolled browser behavior.

Based on the above statement, this PR do:

  • remove the focus prop from the block edit function and replace it with an isSelected prop
  • no need for setFocus prop
  • remove focus information from the state
  • handle all explicit focus calls in the WritingFlow component: When a new block is selected, move the cursor to the right position
  • I kept the focus and setFocus prop for the block's edit function for backwards compatibility concerns now.

This should simplify the block author's work a lot, not need to handle any focus behavior inside blocks.

Testing instructions

This needs heavy testing, all writing flow micro interactions should be tested.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Feb 5, 2018
@youknowriad youknowriad self-assigned this Feb 5, 2018
@youknowriad youknowriad force-pushed the refactor/block-focus branch 2 times, most recently from 83e18d4 to 320b50e Compare February 5, 2018 12:53
@youknowriad
Copy link
Contributor Author

Update: I restored an isSelected prop for the RichText component because we need to show the toolbar of the RichText even if the RichText is not focused sometimes, so a local state for this was not enough.

We may want to revisit this later maybe by checking if the focus is on the RichText it self or one of its attached toolbars, but might not be so easy because of the popovers

@jasmussen
Copy link
Contributor

Thank you for working on this, it seems important. Am I right in guessing that this work might help @iseulde on the mobile focus work?

At first I thought there was an extra big jump when arrow-keying down, but as it turns out that was just because we jumped into a big image and so it had to scroll further:

focus

However one change from master seems to be the block toolbar not showing up when you select text with the keyboard. You have to "shift tab" into the toolbar.

@youknowriad
Copy link
Contributor Author

@jasmussen Yep, this is important as it would really simplify focus management for block authors and will bring more consistency. And true that it may have an impact (and probably simplify) the iOS fix.

I'll take a look to see why the toolbar is not showing up

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

I have a local WIP branch that works on improving undo levels for the RichText component. It makes use of the focus and setFocus props to store and restore the caret position to and from the Redux store. How do you see that working without these props? In other words, we'll need to set caret position programatically, while this change removes this ability.

@youknowriad
Copy link
Contributor Author

@iseulde I don't know but I do think the change here is important enough to try to find an alternative to these functions. These functions were not being used consistently and I think undo/redo should be consistent as well. (What about PlainText, should it have focus and setFocus? what if a block author uses an input?)

About the undo/redo: Can't we update the undo/redo reducer to store a browser selection when adding a undo/redo state and apply this back as part of the WritingFlow component?

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

@youknowriad That sounds good to try. Do you think we can remove the need to pass isSelected to RichText as well? Is there still a need for that?

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

Sorry missed

Update: I restored an isSelected prop for the RichText component because we need to show the toolbar of the RichText even if the RichText is not focused sometimes, so a local state for this was not enough.

@youknowriad
Copy link
Contributor Author

@iseulde I think I'm going to rename it showToolbar or showFormattingControls, it's clearer from the Point of View of the RichText component and it's not always equal to the value of the block's isSelected prop.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

@youknowriad I'm still not understanding the need of this though. The formatting toolbar should only appear when there is RichText focus, toolbars that should appear otherwise should not be put in RichText. When there is no focus, the toolbars have no use anyway.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

Seeing a bug that I don't see in master:

Focus the main content of a quote. The formatting button are at the end of the toolbar. Now select the caption. The formatting buttons moved towards to front.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

To come back to passing selection props to RichText. E.g. create a cover image. Select the block. I don't really expect the formatting buttons to pop up when I don't specifically focus the text.

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

I think this is quite promising. With this we'd get rid of a level of state synchronization between RichText and TinyMCE, which in the short and long runs is always a source of bugs. Furthermore, the way blocks like Quote can be refactored with withState, thus adding an explicit and self-managed extra piece of state, makes much more sense to me.

Tackling all the subtle and less subtle bugs will be challenging, but this route seems worth taking.

@@ -36,7 +37,16 @@ export function BlockEdit( props ) {
// them preferencially as the render value for the block.
const Edit = blockType.edit || blockType.save;

return <Edit { ...props } className={ className } />;
// For backwards compatibility concerns adds a focus and setFocus prop
// These should be removed after some time (maybe when merging to Core)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's worth looking for availability of window.Proxy and, if so, return an instance of it?

focus = new Proxy( {}, { get() { console.warn( 'deprecated' ) } } )
return <Edit  focus={ focus } />
> focus
< Proxy { <target>: {}, <handler>: {…} }

> focus.editable
⚠ deprecated
< undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it 👍

Copy link
Contributor Author

@youknowriad youknowriad Feb 6, 2018

Choose a reason for hiding this comment

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

On a second thought, we'd have to proxify the props object to do so, which I think is not really possible :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I guess it's fine, it seems supported where we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

we'd have to proxify the props object

What do you mean by this? I was just thinking of the following pseudo-code:

if not isSelected: focus = false
else:
  if browserHasProxySupport: focus = deprecatedEmptyObject
  else: focus = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to find a way to proxy any access to props.focus (and not focus.*) because the main use-case for this prop is just checking truthiness to show the block toolbar/inspector

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha.

@@ -105,12 +105,12 @@ class ReusableBlockEdit extends Component {
<BlockEdit
{ ...this.props }
name={ reusableBlock.type }
focus={ isEditing ? focus : null }
isSelected={ isEditing ? isSelected : false }
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally minor, but now that we only deal with booleans we can simplify as { isEditing && isSelected }

### setFocus

// Todo ...

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

const target = this.getInnerTabbable( blockContainer, this.props.initialPosition === -1 );
target.focus();
if ( this.props.initialPosition === -1 ) {
// Special casing richtext because the two functions at the bo bottomare not working as expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Something broke in this comment. :)
  2. I think this branching is partly to blame for some broken behavior when multi-selecting with the keyboard (selection starts at the wrong block, off by one, and often this can be avoided by navigating left and right within a block's RichText).
  3. On the other hand, we get Focus lost when multi-block selection turn into single-block selection #3999 fixed for free. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated with a better comment explaining why we need this special case. in 27f8131

I'm not sure I understand your point 2 here. Are you saying we can avoid this special-case somehow?

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

Seeing a bug that I don't see in master:

Focus the main content of a quote. The formatting button are at the end of the toolbar. Now select the caption. The formatting buttons moved towards to front.

I must have swapped both branches because now I nee it in master, but not in this branch. :D

@youknowriad
Copy link
Contributor Author

youknowriad commented Feb 6, 2018

@iseulde With this proposal (that was my first implementation), If we trigger the "link popover" the focus is not on the RichText anymore, it's on the popover's input so everything disappears.

@@ -385,6 +394,15 @@ export class BlockListBlock extends Component {
}
}

onSelectionChange() {
const selection = window.getSelection();
const range = selection.getRangeAt( 0 );
Copy link
Member

@ellatrix ellatrix Feb 6, 2018

Choose a reason for hiding this comment

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

Seeing an error sometimes:

block.js:399 Uncaught DOMException: Failed to execute 'getRangeAt' on 'Selection': 0 is not a valid index.
    at BlockListBlock.onSelectionChange (http://localhost:8000/wp/wp-content/plugins/gutenberg-dev/editor/build/index.js?ver=1517913835:23499:26)
onSelectionChange @ block.js:399

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not in TinyMCE :)

Do you have a way to consistently reproduce the error? which browser?

Copy link
Member

Choose a reason for hiding this comment

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

We're not in TinyMCE :)

Mixed up components. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, select a paragraph, then click on the border outside the RichText region.

Copy link
Member

Choose a reason for hiding this comment

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

(Chrome)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks this should be fixed now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I just saw that rangeCount is not supported widely yet. Looking for an alternative here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad I'm suddenly getting error on safari regarding const range = getSelection().getRangeAt( 0 ); . It says:

IndexSizeError: DOM Exception 1: Index or size was negative, or greater than the allowed value.

Seems like safari doesn't fully support it yet : https://developer.mozilla.org/en-US/docs/Web/API/Selection/getRangeAt .

Thanks!

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

@iseulde With this proposal (that was my first implementation), If we trigger the "link popover" the focus is not on the RichText anymore, it's on the popover's input so everything disappears.

Good point. Let me think about that. :) In any case we can maybe refine this later.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

@iseulde With this proposal (that was my first implementation), If we trigger the "link popover" the focus is not on the RichText anymore, it's on the popover's input so everything disappears.

I still think this is a case where RichText should handle that internally, since the link pop over is part of that component. It could be something like { ( focus || toolbarFocus ) && <Toolbar /> }?

@youknowriad
Copy link
Contributor Author

youknowriad commented Feb 6, 2018

@iseulde I agree with you conceptually. I just left it out of the scope of this PR for now because it's not easy to do since the links are in Popovers which are not shown as children of the toolbar but in a global popover slot.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

Seeing another issue (I hope I'm not messing up this time):

Pressing enter in the middle of a paragraph moves the caret to the start of the first block.

@ellatrix
Copy link
Member

ellatrix commented Feb 6, 2018

Another one that I don't think I'm seeing in master:

Select some text and create a link. Type a link, press backspace to adjust the typed text, and the whole popover disappears.

@youknowriad youknowriad merged commit 877d497 into master Feb 7, 2018
@youknowriad youknowriad deleted the refactor/block-focus branch February 7, 2018 12:38
@@ -59,6 +51,10 @@ a traditional `input` field, usually when the user exits the field.

*Optional.* By default, all formatting controls are present. This setting can be used to fine-tune formatting controls. Possible items: `[ 'bold', 'italic', 'strikethrough', 'link' ]`.

### `isSelected: Boolean`

*Optional.* Whether to show the input is selected or not in order to show the formatting contros.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: contros -> controls

<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
value={ caption }
focus={ focus }
onFocus={ setFocus }
Copy link
Member

Choose a reason for hiding this comment

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

This change regressed on the fix introduced by #4011, where in that case I explicitly wanted to handle focus within the iframe. Can we otherwise expose manually setting a block as selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like a onSelect prop or something? I'd prefer to avoid it in favor of a global event but if it's not possible for iframes, we could provide it

Copy link
Member

Choose a reason for hiding this comment

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

We could see about generalizing the blur check which occurs in Sandbox, to see if focus has transitioned to an iframe within a block:

componentDidMount() {
window.addEventListener( 'message', this.checkMessageForResize, false );
window.addEventListener( 'blur', this.checkFocus );
this.trySandbox();
}
componentDidUpdate() {
this.trySandbox();
}
componentWillUnmount() {
window.removeEventListener( 'message', this.checkMessageForResize );
window.removeEventListener( 'blur', this.checkFocus );
}
checkFocus() {
if ( this.props.onFocus && document.activeElement === this.iframe ) {
this.props.onFocus();
}
}

@@ -212,6 +252,32 @@ class WritingFlow extends Component {
}
}

componentDidUpdate( prevProps ) {
// When selecting a new block, we focus its first editable or the container
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty jarring for nested blocks when "selecting" occurs by opening "More Options". Looks like we somehow exempted a non-nested block from this behavior?

When trying to open More Options for a nested block which is not currently selected, it forces focus into the first column instead of opening the menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm! why it's not opening the menu even if the focus is moved to the first Column?

I'd expect the order of things to be:

  • Callback to select the block
  • This should rerender and focus the first column
  • Next step in the callback to open the menu (setState)
  • This should rerender and open the menu and moves the focus to the first element in the menu.

I guess the order of things is probably not this one and maybe we can tweak the callback function to match this?

Copy link
Member

Choose a reason for hiding this comment

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

Since the nested block is a different block, it would cause the parent block to become unselected when it's focused.

return;
}

const parentBlock = target.hasAttribute( 'data-block' ) ? target : target.closest( '[data-block]' );
Copy link
Member

@aduth aduth Feb 16, 2018

Choose a reason for hiding this comment

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

FYI: Element#closest includes the node itself in the test, so the ternary is redundant.

The Element.closest() method returns the closest ancestor of the current element (or the current element itself) which matches the selectors given in parameter. If there isn't such an ancestor, it returns null.

https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

document.body.closest( 'body' ) === document.body;
// true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants