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.

Sign up for GitHub

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

Quote Block: Fixing focus transfer between citation and content #5100

Merged
merged 2 commits into from
Feb 16, 2018
Merged

Quote Block: Fixing focus transfer between citation and content #5100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04532b4
Quote Block: Fixing focus transfer between citation and content
youknowriad Feb 16, 2018
9360dc8
Update RichText's content before calling onSplit
youknowriad Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions blocks/library/pullquote/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
}

.wp-block-pullquote {
cite {
display: block;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just increases the click area of the citation to the whole line (width: 100%)

Copy link
Member

Choose a reason for hiding this comment

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

This kind of tells me that we shouldn't be using cite for this... :)

}

cite .blocks-rich-text__tinymce[data-is-empty="true"]:before {
font-size: 14px;
font-family: $default-font;
Expand Down
4 changes: 4 additions & 0 deletions blocks/library/quote/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
.wp-block-quote {
margin: 0;

cite {
display: block;
}
}

.wp-block-quote:not(.is-large) {
Expand Down
4 changes: 3 additions & 1 deletion blocks/library/quote/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ export const settings = {
} )( ( { attributes, setAttributes, isSelected, mergeBlocks, onReplace, className, editable, setState } ) => {
const { align, value, citation, style } = attributes;
const containerClassname = classnames( className, style === 2 ? 'is-large' : '' );
const onSetActiveEditable = ( newEditable ) => () => setState( { editable: newEditable } );
const onSetActiveEditable = ( newEditable ) => () => {
setState( { editable: newEditable } );
};

return [
isSelected && (
Expand Down
26 changes: 21 additions & 5 deletions blocks/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ export class RichText extends Component {
const beforeElement = nodeListToReact( beforeNodes, createTinyMCEElement );
const afterElement = nodeListToReact( afterNodes, createTinyMCEElement );

this.props.onSplit( beforeElement, afterElement );
this.restoreContentAndSplit( beforeElement, afterElement );
} else {
event.preventDefault();
this.onCreateUndoLevel();
Expand Down Expand Up @@ -638,9 +638,10 @@ export class RichText extends Component {

const beforeElement = nodeListToReact( beforeFragment.childNodes, createTinyMCEElement );
const afterElement = nodeListToReact( filterEmptyNodes( afterFragment.childNodes ), createTinyMCEElement );
this.props.onSplit( beforeElement, afterElement, ...blocks );

this.restoreContentAndSplit( beforeElement, afterElement, blocks );
} else {
this.props.onSplit( [], [], ...blocks );
this.restoreContentAndSplit( [], [], blocks );
}
}

Expand Down Expand Up @@ -686,7 +687,7 @@ export class RichText extends Component {
// Splitting into two blocks
this.setContent( this.props.value );

this.props.onSplit(
this.restoreContentAndSplit(
nodeListToReact( before, createTinyMCEElement ),
nodeListToReact( after, createTinyMCEElement )
);
Expand Down Expand Up @@ -740,7 +741,9 @@ export class RichText extends Component {
!! this.editor &&
this.props.tagName === prevProps.tagName &&
this.props.value !== prevProps.value &&
this.props.value !== this.savedContent
this.props.value !== this.savedContent &&
! isEqual( this.props.value, prevProps.value ) &&
! isEqual( this.props.value, this.savedContent )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the real fix. I removed this as part of #4955 but it's necessary to avoid unnecessarily content updates. I don't see any performance loss in adding this and it could even improve performance because we don't use those TinyMCE functions setBookmark and setContent constantly

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad I feel like we removed and added this a few times I the last year now. :) Maybe worth leaving a comment?

Copy link
Member

Choose a reason for hiding this comment

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

(I was surprised to see these lines again.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call 👍

) {
this.updateContent();
}
Expand Down Expand Up @@ -796,6 +799,19 @@ export class RichText extends Component {
} ) );
}

/**
* Calling onSplit means we need to abort the change done by TinyMCE.
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this function. In most places where it's used, we're also calling preventDefault. Shouldn't that be enough to "abort the change" that would otherwise be effected by TinyMCE ?

* we need to call updateContent to restore the initial content before calling onSplit.
*
* @param {Array} before content before the split position
* @param {Array} after content after the split position
* @param {?Array} blocks blocks to insert at the split position
*/
restoreContentAndSplit( before, after, blocks ) {
this.updateContent();
this.props.onSplit( before, after, ...blocks );
Copy link
Member

Choose a reason for hiding this comment

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

Hm, now we have (1) this.props.onSplit checks in several places and (2) none here.

}

render() {
const {
tagName: Tagname = 'div',
Expand Down