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

Block Gallery: deselect first/last image on blur #14930

Merged
merged 3 commits into from
Feb 27, 2020
Merged

Block Gallery: deselect first/last image on blur #14930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e777ba3
Deselect image when none of its children have focus
oandregal Apr 9, 2019
f1aeeb5
Improve comment
oandregal Apr 29, 2019
92a87e4
Update comment
oandregal Nov 19, 2019
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
12 changes: 12 additions & 0 deletions packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class GalleryEdit extends Component {

this.onSelectImage = this.onSelectImage.bind( this );
this.onSelectImages = this.onSelectImages.bind( this );
this.onDeselectImage = this.onDeselectImage.bind( this );
this.setLinkTo = this.setLinkTo.bind( this );
this.setColumnsNumber = this.setColumnsNumber.bind( this );
this.toggleImageCrop = this.toggleImageCrop.bind( this );
Expand Down Expand Up @@ -122,6 +123,16 @@ class GalleryEdit extends Component {
};
}

onDeselectImage( index ) {
return () => {
if ( this.state.selectedImage === index ) {
this.setState( {
selectedImage: null,
} );
}
};
}

onMove( oldIndex, newIndex ) {
const images = [ ...this.props.attributes.images ];
images.splice( newIndex, 1, this.props.attributes.images[ oldIndex ] );
Expand Down Expand Up @@ -427,6 +438,7 @@ class GalleryEdit extends Component {
onMoveForward={ this.onMoveForward }
onRemoveImage={ this.onRemoveImage }
onSelectImage={ this.onSelectImage }
onDeselectImage={ this.onDeselectImage }
onSetImageAttributes={ this.setImageAttributes }
onFocusGalleryCaption={ this.onFocusGalleryCaption }
/>
Expand Down
38 changes: 37 additions & 1 deletion packages/block-library/src/gallery/gallery-image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { debounce } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -25,11 +26,26 @@ class GalleryImage extends Component {
constructor() {
super( ...arguments );

this.onBlur = this.onBlur.bind( this );
this.onFocus = this.onFocus.bind( this );
this.onSelectImage = this.onSelectImage.bind( this );
this.onSelectCaption = this.onSelectCaption.bind( this );
this.onRemoveImage = this.onRemoveImage.bind( this );
this.bindContainer = this.bindContainer.bind( this );

// The onDeselect prop is used to signal that the GalleryImage component
// has lost focus. We want to call it when focus has been lost
// by the figure element or any of its children but only if
// the element that gained focus isn't any of them.
//
// debouncedOnSelect is scheduled every time a figure's children
// is blurred and cancelled when any is focused. If none gain focus,
// the call to onDeselect will be executed.
//
// onBlur / onFocus events are quick operations (<5ms apart in my testing),
// so 50ms accounts for 10x lagging while feels responsive to the user.
this.debouncedOnDeselect = debounce( this.props.onDeselect, 50 );
Copy link
Member

Choose a reason for hiding this comment

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

I still don't get from this comment why focus on the same element would be called right after blur. In what circumstances does rapid focus & blur happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the comment to better explain the mechanism: the idea is to call onDeselect when focus has transitioned to a different React component (another GalleryImage, toolbar, etc.) but not when focus transitions within the DOM elements that form this React component (figure, image, caption, toolbar, etc).

Does it make more sense now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ellatrix any chance you'd be able to review this and see if the comment makes more sense now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we instead just check that the window.activeElement property is not a children of the wrapper (using a ref) instead of this magic trick :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just code! Perhaps the comment is more confusing than the code itself? Suggestions welcome :)

I had tried the comparison with the activeElement, but the global focus handler won't be reliably dispatched, so I resorted to the focus events dispatched by the children instead.

Copy link
Member Author

@oandregal oandregal Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I still don't get from this comment why focus on the same element would be called right after blur. In what circumstances does rapid focus & blur happen?

Now that I revisit this thread I think I can explain this better! It's not that rapid focus and blur happen on the same element, but that focus and blur on children element bubble to figure. In the DOM, blur and focus don't bubble, but React Events mechanism normalizes all events to bubble - even blur and focus. So this is how events work in gallery:

  • user clicks image => focus event is fired with image as target then it bubbles to figure
  • user tabs to figcaption =>
    • blur event is fired for image as target, then it bubbles to figure
    • focus event is fired for figcaption, then it bubbles to figure

And the same thing for the GalleryImage buttons (icons to remove or move the image). So, instead of tracking blur events individually, this PR tracks them in a single place.

Also worth noting that if you check the activeElement in a blur event it'll be body in Firefox and Chrome (it doesn't give you the next element to focus). So, the only alternative I see would be to listen to focus events at the window level, and check if the activeElement is one of the GalleryImage children. This doesn't work because the focus won't bubble in some situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nosolosw Could you do something like event.currentTarget.contains( event.relatedTarget ) in the onBlur to detect when focus moved outside a gallery image?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would work and it seems to have good browser support. However, the feature is marked as experimental and is only in a working draft. My gut feeling is that we shouldn't use it just yet, what do you think?


this.state = {
captionSelected: false,
};
Expand Down Expand Up @@ -103,6 +119,22 @@ class GalleryImage extends Component {
}
}

/**
* Note that, unlike the DOM, all React events bubble,
* so this will be called after the onBlur event of any figure's children.
*/
onBlur() {
this.debouncedOnDeselect();
}

/**
* Note that, unlike the DOM, all React events bubble,
* so this will be called after the onBlur event of any figure's children.
oandregal marked this conversation as resolved.
Show resolved Hide resolved
*/
onFocus() {
this.debouncedOnDeselect.cancel();
}

render() {
const {
url,
Expand Down Expand Up @@ -159,7 +191,11 @@ class GalleryImage extends Component {
} );

return (
<figure className={ className }>
<figure
className={ className }
onBlur={ this.onBlur }
onFocus={ this.onFocus }
>
{ href ? <a href={ href }>{ img }</a> : img }
<div className="block-library-gallery-item__move-menu">
<Button
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/gallery/gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const Gallery = ( props ) => {
onMoveForward,
onRemoveImage,
onSelectImage,
onDeselectImage,
onSetImageAttributes,
onFocusGalleryCaption,
} = props;
Expand Down Expand Up @@ -78,6 +79,7 @@ export const Gallery = ( props ) => {
onMoveForward={ onMoveForward( index ) }
onRemove={ onRemoveImage( index ) }
onSelect={ onSelectImage( index ) }
onDeselect={ onDeselectImage( index ) }
setAttributes={ ( attrs ) =>
onSetImageAttributes( index, attrs )
}
Expand Down