Visual Editor: Keep track of the focused block #438

youknowriad · 2017-04-17T12:58:17Z

references #420

In this PR, I'm adding the focus state to keep track of the focused block and any extra config that let us focus the right element. For now, I'm storing the focused block and the focused editable inside the block. To make undo/redo work correctly, we'll need to add the position inside the focused editable.

closes #437

youknowriad · 2017-04-17T12:59:06Z

blocks/library/image/index.js


+		/* eslint-disable */


the onClick adds to many accessibility linting errors :(

Not sure of the context, but is it worth adding a note about this to a ticket like #392?

Also, we should be specifically disable only the lint rule we want to target, in case there are other lint issues in the region.

I like the idea of commenting on any disable we apply, since it holds us accountable to understanding why lint errors occur and how they're not applicable.

Yep that was my intent, but this is breaking three rules :). I'll update.

aduth

This seems reasonable to me. As with anything we add, we should be conscious of the developer experience impact. I think @iseulde raises a good point at #420 (comment) . At a glance, "focused" and "selected" seem synonymous, so how can we clarify or consolidate the two? With regards to new props being passed to edit, are there options to explore with React's hidden context feature to help avoid the need for the block implementer to consider managing focus themselves?

aduth · 2017-04-17T16:30:06Z

editor/state.js

+ * @param  {Object} action Dispatched action
+ * @return {Object}        Updated state
+ */
+export function focus( state = {}, action ) {


Should this be included in undo history?

Probably 👍

Tried this and it has several implications, we should ignore consecutive focuses etc... I think this should be added but in its own PR.

aduth · 2017-04-17T16:32:42Z

blocks/library/image/index.js


+		/* eslint-disable */


Also, we should be specifically disable only the lint rule we want to target, in case there are other lint issues in the region.

I like the idea of commenting on any disable we apply, since it holds us accountable to understanding why lint errors occur and how they're not applicable.

aduth · 2017-04-17T16:34:28Z

blocks/library/image/index.js

 					<Editable
 						tagName="figcaption"
 						placeholder={ wp.i18n.__( 'Write caption…' ) }
 						value={ caption }
+						focus={ focus && focus.editable === 'caption' ? focus : null }


Do we even need the config here since we only have one focusable element? Wondering if focusCaption could be implemented just as:

const focusCaption = () => setFocus();

And we'd infer caption should be focused merely by a truthy incoming focus value?

I hesitated and you're probably right here.

aduth · 2017-04-17T16:35:33Z

blocks/library/quote/index.js

@@ -7,7 +7,7 @@ import Editable from 'components/editable';

 const { parse, html, query } = hpq;

-const fromValueToParagraphs = ( value ) => value.map( ( paragraph ) => `<p>${ paragraph }</p>` ).join( '' );
+const fromValueToParagraphs = ( value ) => value ? value.map( ( paragraph ) => `<p>${ paragraph }</p>` ).join( '' ) : '';


Is this fixing a bug? Do we really want to support an undefined value? Do we need some concept of default attributes, whether that's an explicit property of a block or even just how we destructure attributes in edit and save?

Yes this fixes the bug of inserting an empty quote block (using the inserter) since we're assuming the attributes are all nullable.

youknowriad · 2017-04-17T17:13:30Z

@aduth I'm not sure how can we prevent the block author to handle the focus

youknowriad · 2017-04-17T17:14:38Z

I think @iseulde raises a good point at #420 (comment) . At a glance, "focused" and "selected" seem synonymous, so how can we clarify or consolidate the two?

I agree, and I'm really bad with naming things. Open to any suggestion.

aduth · 2017-04-17T19:07:43Z

@aduth I'm not sure how can we prevent the block author to handle the focus

Well, at first I'd thought to use React's context to have the visual editor define child context values that the Editable component could directly access. This is how react-redux gets around the need to pass store as a prop down through intermediary components. But thinking about react-redux got me to consider whether it could interact with the store directly? The obvious problem is that Editable is supposed to not be aware of the editor, being that it's a general purpose component in the blocks directory. Per #408, I think we'll need to find a better solution that lets us treat components as editor-aware while still exposing them for consumption to external plugins.

aduth · 2017-04-17T20:38:25Z

I agree, and I'm really bad with naming things. Open to any suggestion.

I'm not totally sure it's entirely a naming problem, as I don't really see much of a conceptual difference between our idea of selected and focused. Instead, it seems that "selected" is different only in that it accounts for typing status to fade out the border and controls. And if that's the case (correct me if it's not), could this be better reflected by a standalone isTyping state value updated in response to a global keyboard event?

youknowriad · 2017-04-18T10:19:21Z

@aduth @iseulde Good point.

In e3ce9f6 I merged the "selected" and the "focused" concepts to avoid confusion.

@aduth I'm not certain we should hide the "focus" management from the block author. I think the block author should have some control on keyboard navigation and stuff like that (think moving the cursor from the content to the citation in the quote block). I think we should merge this as is and explore enhancements.

aduth · 2017-04-19T12:39:43Z

editor/modes/visual-editor/block.js

-			onKeyDown={ onDeselect }
-			onMouseEnter={ onMouseEnter }
+			onKeyDown={ onStartTyping }
+			onMouseMove={ onMouseMove }


onMouseMove is an incredibly noisy event (watch Redux DevTools while moving). Is the idea here that we want the block to regain its hover effects when moving the mouse while typing? Maybe we can at least add a condition to only trigger the callback if isTyping is true?

Exactly! yep good ideal to limit it to when we're typing.

aduth · 2017-04-19T12:40:41Z

editor/modes/visual-editor/block.js

 				attributes={ block.attributes }
-				setAttributes={ setAttributes } />
+				setAttributes={ setAttributes }
+				updateFocus={ onFocus }


Even just for consistency with setAttributes, do you think naming this setFocus is still accurate / any better?

aduth · 2017-04-19T12:41:41Z

editor/modes/visual-editor/block.js

@@ -122,6 +131,13 @@ export default connect(
 				hovered: false,
 				uid: ownProps.uid
 			} );
+		},
+		onFocus( config = {} ) {


Should we be assigning the default config into state memory or would it be more appropriate to apply the default in mapStateToProps?

Right, handling the default value in the state is safer

aduth · 2017-04-19T12:42:48Z

editor/state.js

+			};
+
+		case 'START_TYPING':
+			if ( action.uid !== state.uid ) {


Does typing consideration need to be per-block? Seems like a global effect.

Like implemented for now "START_TYPING" is per-block. It's triggered when typing inside a specific block, we can drop the uid. I feel it should stay as is unless we catch the start typing in a higher level maybe?

aduth · 2017-04-19T12:43:46Z

Needs a rebase.

ellatrix · 2017-04-19T15:28:08Z

If it helps, in previous prototypes I had also a separate key for showing and hiding UI in the Redux store. SHOW_UI on mousedown touchstart focus and HIDE_UI on keydown blur. There are state checks before dispatching.

youknowriad · 2017-04-19T15:59:50Z

Feedback addressed, should we merge?

aduth · 2017-04-19T16:08:11Z

blocks/components/editable/index.js

-		} else if ( this.props.value !== prevProps.value ) {
+		}
+
+		if ( this.props.focus !== prevProps.focus && this.props.focus ) {


Because of the strict comparison of objects and how the reducer is creating next state, this condition passes much more frequently than we'd probably expect. For example I see upon adding a console.log to the inner block that it gets called twice every time I select a block.

Maybe if ( this.props.focus && ! prevProps.focus ) would be better? Edit: no, looks like I probably misunderstood the intent here.

I think to avoid these calls, we could check for a falsy this.props.focus before calling the onFocus prop for now. Once we introduce the focus position (probably a following PR) we should check if it's the same position before calling the callback.

@nylen proposition is valid for now because we don't store any offset yet. I updated in 7488f23.

nylen · 2017-04-19T16:30:56Z

What is this.props.focus? What are its possible valid values and what does each one mean? How can we make this more clear?

Why is it an object rather than a primitive such as a boolean (or a string to indicate that a specific field is focused)? If it's possible to make it a primitive, this would also solve #438 (comment) at the same time.

nylen · 2017-04-19T16:33:15Z

Also, from a usability perspective, this seems to be working extremely well and addresses a bunch of issues I was having previously, so huge 👍 there

youknowriad · 2017-04-19T16:38:07Z

@nylen this.props.focus is a nullable object. If it's falsy, this means the current block does not have the focus. If it's not, its shape is defined partially in this PR. and the final shape could be { editable: 'citation', offset: 10 } (maybe tinymce's bookmark instead of offset, not sure yet).

The editable is optional. It's intended for blocks with multiple editables and the offset is the offset inside this editable.

nylen · 2017-04-19T16:40:18Z

OK, thanks for explaining. How can we document this in the code? Should it be two separate properties instead?

Probably the solution to #438 (comment) then is to compare each of the primitive properties inside of focus.

youknowriad · 2017-04-20T08:53:19Z

Feedback addressed, anyone to give a last 👀 before merging?

nylen

As Riad pointed out in Slack, changing focus to primitive properties would complicate the code. We can document afterwards along with the rest of the edit parameters. I'll make a separate issue for that.

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 17, 2017

youknowriad self-assigned this Apr 17, 2017

youknowriad requested review from aduth and ellatrix April 17, 2017 12:58

youknowriad commented Apr 17, 2017

View reviewed changes

youknowriad mentioned this pull request Apr 17, 2017

Editable: Enter splits the current paragraph into two blocks #409

Merged

youknowriad force-pushed the add/focus-state branch from 598fe5b to cea6e0a Compare April 17, 2017 15:52

aduth reviewed Apr 17, 2017

View reviewed changes

aduth reviewed Apr 19, 2017

View reviewed changes

youknowriad force-pushed the add/focus-state branch from e3ce9f6 to 5b53075 Compare April 19, 2017 13:01

aduth reviewed Apr 19, 2017

View reviewed changes

youknowriad added 5 commits April 20, 2017 14:34

Visual Editor: Keep track of the focused block

7a93da4

Handling the focus in the quote block

5686bd8

Disabling specific ESLint rules

004ef08

Merge the focus and the selected contcepts

6db8d8e

Focus: rename updateFocus to setFocus and default value in state

c990b51

Optimize onMouseMove calls

e0cefb8

youknowriad force-pushed the add/focus-state branch from 7488f23 to 35a7e2d Compare April 20, 2017 13:43

Optimizing the updating focus call

56af22c

youknowriad force-pushed the add/focus-state branch from 35a7e2d to 56af22c Compare April 20, 2017 14:23

nylen approved these changes Apr 20, 2017

View reviewed changes

youknowriad merged commit c970c16 into master Apr 20, 2017

youknowriad deleted the add/focus-state branch April 20, 2017 16:22

nylen mentioned this pull request Apr 20, 2017

Document all Block API functions #468

Closed

Visual Editor: Keep track of the focused block #438

Visual Editor: Keep track of the focused block #438

Conversation

youknowriad commented Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Apr 17, 2017

youknowriad commented Apr 17, 2017

aduth commented Apr 17, 2017

aduth commented Apr 17, 2017 • edited Loading

youknowriad commented Apr 18, 2017

Choose a reason for hiding this comment

youknowriad Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 19, 2017

ellatrix commented Apr 19, 2017 • edited Loading

youknowriad commented Apr 19, 2017

Choose a reason for hiding this comment

nylen Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Apr 19, 2017 • edited Loading

nylen commented Apr 19, 2017

youknowriad commented Apr 19, 2017

nylen commented Apr 19, 2017

youknowriad commented Apr 20, 2017

nylen left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 17, 2017 •

edited

Loading

aduth commented Apr 17, 2017 •

edited

Loading

youknowriad Apr 19, 2017 •

edited

Loading

ellatrix commented Apr 19, 2017 •

edited

Loading

nylen Apr 19, 2017 •

edited

Loading

nylen commented Apr 19, 2017 •

edited

Loading