Focal Point Picker by jeffersonrabb · Pull Request #10925 · WordPress/gutenberg

jeffersonrabb · 2018-10-22T23:02:04Z

Description

This PR creates a Focal Point Picker component and implements it in the Cover block. Here is the rationale for this functionality: when dealing with large background images like the ones used in Cover, it's common to see undesirable crops, especially on smaller viewports. If the primary subject of an image happens to be dead center then it will display perfectly everywhere, but if it is off-center by even a little, key visual information can be cropped out.

The Focal Point Picker component allows the user to visually select the ideal center point, which is then returned as a pair of percentage-like coordinates (e.g. { x: 0.1, y: 0.9 }) which can easily be converted into background-position attributes and applied to the element with the background image.

Video and Parallax

The Focal Point Picker only works if an image is being used, and if Fixed Background is disabled. For now, the same approach won't be applied to videos, and Parallaxed images don't really require a focal point since their crop changes as the user scrolls.

Legacy Content

As part of this branch, Cover block saves the dimensions of images when selected, which is needed by the component. For instances of Cover block that pre-date this branch there will be no dimensions data, and therefore the focal point picker is suppressed. Because of this approach there was no need to add a Gutenberg deprecation definition.

How has this been tested?

Spin up local Gutenberg instance following instructions here: https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md
Switch branch to try/focal-point-picker
Create new post
Add Cover block to post.
Upload or select a large image, ideally one where the central focus is not off center - the further the better.
Click on block, then show block settings. In the sidebar, click and drag the picker icon to the
focal point of the image. Depending on the dimensions, you will probably see the image move around a little in the editor.
Save the post and preview.

Screenshots

Here are two screen captures demonstrating the use of the focal point picker:

https://cloudup.com/cIIvWAafA20

https://cloudup.com/cnL3p-eZvSq

Types of changes

The Focal Point Picker is a new feature, which shouldn't affect anything, however the implementation in Cover could possibly break the block and should be tested thoroughly. Another important testing flow is to create a Cover block while on master, then branch switch to try/focal-point-picker. The block should remain exact as it was, with no focal point picker visible.

Checklist:

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

mtias · 2018-10-23T10:31:55Z

Thanks @jeffersonrabb for the proposal and implementation! I think this will be a neat addition. Having it as a reusable component would also help others build blocks with equal capabilities (and a consistent experience).

MichaelArestad · 2018-12-21T18:32:15Z

This is great!

designsimply · 2019-01-16T16:28:23Z

@jasmussen, @jeffersonrabb is going to rebase and I was wondering if you might be able to help out with a design feedback review?

…al Point Picker for non-parallaxed image backgrounds.

…ess focal point picker UI if image dimensions aren't available, 2) suppress background-position styling unless focal point is valid.

…e.scss instead of the components index.s.

jeffersonrabb · 2019-01-16T20:09:16Z

Rebased and ready for review!

gziolo · 2019-01-17T05:24:08Z

+} );
+
+/* Example function to render the CSS styles based on Focal Point Picker value */
+const renderImageContainerWithFocalPoint = ( url, focalPoint ) => {


How would this function be used? It’s not clear from this document.

This is meant to illustrate how the percentage values from the component can be translated into a "real-world" CSS background-position definition. The relevant line in Cover block is:

gutenberg/packages/block-library/src/cover/index.js

Line 241 in ded1017

style.backgroundPosition = `${ focalPoint.x * 100 }% ${ focalPoint.y * 100 }%`;

It would probably make sense for me to use the actual example instead of the hypothetical, but I'd prefer to hold off on updating the README until the PR is ready to merge, in case there are changes still to come. Would this be a reasonable approach?

gziolo · 2019-01-17T05:26:40Z

+		y: 0.5
+	},
+} )( ( { focalPoint, setState } ) => { 
+	const url = '/path/to/image';


This example is going to be used in auto-generated docs so it should ideally provide a valid url that can be rendered.

Are there examples of valid url parameters in any other Gutenberg components? I just did a quick scan looking for one and came up blank, but maybe you can point me to a relevant one?

See how ResponsiveWrapper is documented:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/responsive-wrapper/README.md

This is how it renders in Calypso DevDocs:
https://wpcalypso.wordpress.com/devdocs/gutenberg-components/responsive-wrapper

gziolo · 2019-01-17T05:28:42Z

+import { withInstanceId, compose } from '@wordpress/compose';
+import BaseControl from '../base-control/';
+import withFocusOutside from '../higher-order/with-focus-outside';
+import classnames from 'classnames';


Only classnames is an external dependency here.

Packages with the WordPress namespace should be in their own group.

Everything else is internal dependencies.

Resolved in 97fcd04

gziolo · 2019-01-17T05:33:38Z

I didn’t do very in-depth review of this PR, just initial pass. At first glance, it looks good. Thanks for working on it 👍

youknowriad · 2019-01-18T08:19:35Z

+	},
+	dimensions: {
+		type: 'object',
+	},


Why do we need to store the dimensions? Can't we just use percentage in the focal point instead?

The focal point picker needs image dimensions for two reasons:

To define the drag area for the target. The user should only be able to drag the target over the area of the image, and unless the image's aspect ratio precisely matches the focal point picker's area there will be blank space on the left/right or top/bottom. Awareness of the dimensions allows a precise definition of the drag area. This is done here:

gutenberg/packages/components/src/focal-point-picker/index.js

Lines 39 to 64 in 97fcd04

calculateBounds() {

const { dimensions } = this.props;

const pickerDimensions = this.pickerDimensions();

const bounds = {

top: 0,

left: 0,

bottom: 0,

right: 0,

width: 0,

height: 0,

};

const widthRatio = pickerDimensions.width / dimensions.width;

const heightRatio = pickerDimensions.height / dimensions.height;

if ( heightRatio >= widthRatio ) {

bounds.width = bounds.right = pickerDimensions.width;

bounds.height = dimensions.height * widthRatio;

bounds.top = ( pickerDimensions.height - bounds.height ) / 2;

bounds.bottom = bounds.top + bounds.height;

} else {

bounds.height = bounds.bottom = pickerDimensions.height;

bounds.width = dimensions.width * heightRatio;

bounds.left = ( pickerDimensions.width - bounds.width ) / 2;

bounds.right = bounds.left + bounds.width;

}

return bounds;

}

To convert the location of the target to a percentage-based focal point we need to know the dimensions of the original image. This calculation occurs here:

gutenberg/packages/components/src/focal-point-picker/index.js

Lines 65 to 90 in 97fcd04

onMouseMove( event ) {

const { isDragging, bounds } = this.state;

const { onChange } = this.props;

if ( isDragging ) {

const pickerDimensions = this.pickerDimensions();

const cursorPosition = {

left: event.pageX - pickerDimensions.left,

top: event.pageY - pickerDimensions.top,

};

const left = Math.max(

bounds.left, Math.min(

cursorPosition.left, bounds.right

)

);

const top = Math.max(

bounds.top, Math.min(

cursorPosition.top, bounds.bottom

)

);

onChange( {

x: left / pickerDimensions.width,

y: top / pickerDimensions.height,

} );

}

}

However, I don't love requiring dimensions as a parameter for the component. First of all, it's restrictive - ideally the component just takes the image path and handles the rest. And second, it also creates a deprecation problem for Cover blocks created prior to the Focal Point Picker in which the dimensions are not retained. Ideally, I'd prefer that the Focal Point Picker component determine the dimensions programmatically from the image itself. I'm going to experiment with a few approaches to this and will update the PR shortly.

In 5567d16 I've introduced a different approach to getting the image dimensions. The parameters are gone. Instead, the component creates an invisible <img /> (opacity: 0) and derives the dimensions from that. This way the block using the component need not supply dimensions and the component will work with legacy blocks. Does this seem like a viable approach?

Update: as of b67aaa4 I've removed the background image completely in favor of the IMG element, so it is no longer invisible as I indicated above.

jasmussen · 2019-01-18T09:04:41Z

Really nice work.

GIF:

Can you make the focal point picker look like this?

That is, $blue-medium-600 for the spot color, 1px white stroke around it so it's visible on all backgrounds, and 2px thick circle. This makes it feel related to the resize handles on images:

There's white padding above and below the focal point picker — is this intended? Can it be removed?

jasmussen · 2019-01-18T09:09:13Z

Can you add input fields below the picker that matches the percentages typed in? This would be an accessible option:

…s, for accessibility reasons. Slight corrections to the postion calculations.

jeffersonrabb · 2019-01-18T17:26:38Z

Can you add input fields below the picker that matches the percentages typed in? This would be an accessible option:

Done in 7131d07. I added my own styles for placing the two TextControls side-by-side. If there is a Gutenberg component or other structure that makes this easier to do, let me know and I will replace. Also note that these text fields are not editable. Making them editable would introduce a bit more complexity - is this needed?

jasmussen · 2019-01-21T09:58:09Z

With the recent changes, this is good to go from a design & accessibility perspective. Both could use sanity checks, of course, but thumbs up from me.

youknowriad

Nice work here, left some minor notes.

kjellr · 2019-01-21T15:02:49Z

With the recent changes, this is good to go from a design & accessibility perspective.

+1 to this. The only small issue I'm seeing now is there are a lot of selection artifacts left over when I drag the focal point picker around in Safari:

Not sure if there's anything we can do about that.

jeffersonrabb · 2019-01-21T15:08:06Z

The only small issue I'm seeing now is there are a lot of selection artifacts left over when I drag the focal point picker around in Safari:

Before I dig in on this, do you happen to recall if the issue was occurring last week, before @jasmussen replaced the target SVG?

jasmussen · 2019-01-21T15:17:04Z

Oh hey, one thing springs to mind here. There were some translate styles baked into the SVG background I replaced, those almost certainly have something to do with it:

1bd8ea0#diff-1315a70baa9fba9ba6182808f2bbe75bL1

translate(1 1)

Not sure what that does, specifically, but I'm familiar with an old hack where we apply transform: translateZ(0) on an object we want to be painted by the GPU for performance. This is likely what that old rule did as well, so perhaps worth trying. @kjellr does that ring any bells?

Also noting that will-change: transform; should accomplish the same — render the element on the GPU — but I can't recall if support for that is great yet or not.

kjellr · 2019-01-21T15:43:03Z

Oh hey, one thing springs to mind here. There were some translate styles baked into the SVG background I replaced, those almost certainly have something to do with it:

1bd8ea0#diff-1315a70baa9fba9ba6182808f2bbe75bL1

translate(1 1)

Not sure what that does, specifically, but I'm familiar with an old hack where we apply transform: translateZ(0) on an object we want to be painted by the GPU for performance. This is likely what that old rule did as well, so perhaps worth trying. @kjellr does that ring any bells?

Yep, that's it. Adding translateZ(0) or will-change: transform; eliminates the issue.

…onvention. Moved render() function to be the last one.

jeffersonrabb · 2019-01-21T20:05:41Z

Yep, that's it. Adding translateZ(0) or will-change: transform; eliminates the issue.

Added will-change:transform; in b44f1b9

Seems to work well.

kjellr · 2019-01-21T20:21:12Z

Added will-change:transform; in b44f1b9

Seems to work well.

Can confirm — I'm no longer seeing those artifacts.

I think we're all set from a design perspective now. 👍

jasmussen · 2019-01-28T14:22:13Z

🎉

afercia · 2019-02-15T14:02:58Z

I've just noticed this component renders two input fields that are completely unlabelled. The visible text is just plain text.

There's no associated <label> element or aria-label attribute to give these two controls a proper label. I'm going to create an issue.

Similar cases happened several times in the past months and I'm sorry to have to point it out again: this is a very basic accessibility requirement. There's been some discussion in the past about a way to try to avoid this kind of oversights but nothing concrete happened. Please, let's try to find together a way to improve the process. 🙂

aduth · 2019-02-15T14:14:31Z

+					</div>
+				</div>
+				<div className="components-focal-point-picker_position-display-container">
+					<BaseControl label={ __( 'Horizontal Pos.' ) }>


Per @afercia 's comment, I think this needed to have been assigned an id for a label to be produced associated with the rendered input (which does have an id).

It seems at some point BaseControl was updated to be accommodating to receiving a label and not an id, at which point it renders a span. I don't know that this should be supported, as it leaves open the possibility for errors like this.

gutenberg/packages/components/src/base-control/index.js

Line 11 in 858719b

{ label && ! id && <span className="components-base-control__label">{ label }</span> }

Thanks @aduth I think that change was made for the new font size picker:

where the visible text can't be a label element. At that point though, the input child must have an aria-label attribute: and yes, this leaves open the possibility that developers forget or don't know they need to label the input.

where the visible text can't be a label element

(Related: #9802 (comment) )

Forgive my understanding, but it's not clear to me why in that example "Font Size" would not be a label element associated with the dropdown input?

Not all elements can have an associated element. Form elements and a few others can. What is the dropdown input in this case? The div?

Not all elements can have an associated element. Form elements and a few others can. What is the dropdown input in this case? The div?

There's a button which could receive the initial focus for the dropdown.

Yes, technically a button is a labelable element, here's a list: https://www.w3.org/TR/html52/sec-forms.html#labelable-element

However, it's not just about click > focus. The primary goal of labels is giving names to controls. In this case, it wouldn't make sense. A <label> element would override the button text. It wouldn't override the button aria-label though, which is, for example: aria-label: "Font size: Normal". It would be a very uncommon usage. Seems to me your proposal in #9802 (comment) is preferable.

FYI these specific BaseControl were corrected by @jorgefilipecosta in #14152

swissspidy · 2019-02-25T15:18:12Z

Why wasn't the addition of this new component reflected in the changelog?

aduth · 2019-02-25T15:25:20Z

Why wasn't the addition of this new component reflected in the changelog?

It should have been, but the changelog workflows are not universally adopted / enforced. This should become less an issue after #13594.

mtias added [Type] Enhancement A suggestion for improvement. [Feature] Media Anything that impacts the experience of managing media Needs Design Feedback Needs general design feedback. labels Oct 23, 2018

mtias added this to the Future: Phase 2 milestone Nov 12, 2018

jeffersonrabb added 8 commits January 16, 2019 14:06

Focal Point Picker, initial commit. Cover block implementation of Foc…

684e09b

…al Point Picker for non-parallaxed image backgrounds.

Focal Point Picker README, initial commit.

397ea7b

Fixes for Cover blocks that pre-date the Focal Point Picker: 1) suppr…

2d6d651

…ess focal point picker UI if image dimensions aren't available, 2) suppress background-position styling unless focal point is valid.

Small mistake in sample code.

4958adc

README edits.

b121c90

Missing file.

e019989

Include Focal Point Picker stylesheet in packages/components/src/styl…

021888c

…e.scss instead of the components index.s.

Some conflict resolution in cover block's index.js.

ded1017

jeffersonrabb force-pushed the try/focal-point-picker branch from c1ecc3b to ded1017 Compare January 16, 2019 20:07

jeffersonrabb requested review from a team and jasmussen January 16, 2019 20:10

gziolo modified the milestones: Future: Phase 2, 5.0 (Gutenberg) Jan 17, 2019

gziolo requested a review from karmatosed January 17, 2019 05:17

gziolo reviewed Jan 17, 2019

View reviewed changes

Regrouping of component dependencies.

97fcd04

youknowriad reviewed Jan 18, 2019

View reviewed changes

Display horizontal and vertical positions as percentages in text boxe…

7131d07

…s, for accessibility reasons. Slight corrections to the postion calculations.

youknowriad reviewed Jan 21, 2019

View reviewed changes

Comment thread packages/components/src/focal-point-picker/index.js Outdated

Comment thread packages/components/src/focal-point-picker/index.js Outdated

jeffersonrabb added 2 commits January 21, 2019 14:53

CSS tweak to avoid visual artifacts in Safari during dragging.

b44f1b9

Changed "component" prefix for all classes to "components" to match c…

d61d15a

…onvention. Moved render() function to be the last one.

youknowriad added the Needs Accessibility Feedback Need input from accessibility label Jan 28, 2019

youknowriad approved these changes Jan 28, 2019

View reviewed changes

youknowriad merged commit 77f6176 into master Jan 29, 2019

youknowriad deleted the try/focal-point-picker branch January 29, 2019 07:32

This was referenced Feb 4, 2019

Bump plugin version to 5.0.0-rc.1 #13652

Merged

Bump plugin version to 5.0.0 #13689

Closed

Bump plugin version to 5.0.0 #13692

Merged

jameskoster mentioned this pull request Feb 6, 2019

Add FocalPointPicker component to Featured Product block woocommerce/woocommerce-blocks#403

Closed

youknowriad mentioned this pull request Feb 11, 2019

Phase 2 Scope & Features #13113

Closed

22 tasks

afercia mentioned this pull request Feb 15, 2019

FocalPointPicker renders unlabelled input fields #13894

Closed

aduth reviewed Feb 15, 2019

View reviewed changes

afercia mentioned this pull request Feb 15, 2019

Updates to the Font Size Picker #9802

Merged

2 tasks

SchneiderSam mentioned this pull request Feb 16, 2019

Add Block: Section #4900

Closed

kjetisve mentioned this pull request Feb 28, 2019

Allow theme to control sizes attribute for wide/full images #6131

Closed

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Focal Point Picker for the Cover Block (#10925)

cb4bc37

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Focal Point Picker for the Cover Block (#10925)

e446a12

	calculateBounds() {
	const { dimensions } = this.props;
	const pickerDimensions = this.pickerDimensions();
	const bounds = {
	top: 0,
	left: 0,
	bottom: 0,
	right: 0,
	width: 0,
	height: 0,
	};
	const widthRatio = pickerDimensions.width / dimensions.width;
	const heightRatio = pickerDimensions.height / dimensions.height;
	if ( heightRatio >= widthRatio ) {
	bounds.width = bounds.right = pickerDimensions.width;
	bounds.height = dimensions.height * widthRatio;
	bounds.top = ( pickerDimensions.height - bounds.height ) / 2;
	bounds.bottom = bounds.top + bounds.height;
	} else {
	bounds.height = bounds.bottom = pickerDimensions.height;
	bounds.width = dimensions.width * heightRatio;
	bounds.left = ( pickerDimensions.width - bounds.width ) / 2;
	bounds.right = bounds.left + bounds.width;
	}
	return bounds;
	}

	onMouseMove( event ) {
	const { isDragging, bounds } = this.state;
	const { onChange } = this.props;

	if ( isDragging ) {
	const pickerDimensions = this.pickerDimensions();
	const cursorPosition = {
	left: event.pageX - pickerDimensions.left,
	top: event.pageY - pickerDimensions.top,
	};
	const left = Math.max(
	bounds.left, Math.min(
	cursorPosition.left, bounds.right
	)
	);
	const top = Math.max(
	bounds.top, Math.min(
	cursorPosition.top, bounds.bottom
	)
	);
	onChange( {
	x: left / pickerDimensions.width,
	y: top / pickerDimensions.height,
	} );
	}
	}

Conversation

jeffersonrabb commented Oct 22, 2018

Description

Video and Parallax

Legacy Content

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

mtias commented Oct 23, 2018

Uh oh!

MichaelArestad commented Dec 21, 2018

Uh oh!

designsimply commented Jan 16, 2019

Uh oh!

jeffersonrabb commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Jan 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffersonrabb Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Jan 18, 2019

Uh oh!

jasmussen commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffersonrabb commented Jan 18, 2019

Uh oh!

jasmussen commented Jan 21, 2019

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kjellr commented Jan 21, 2019

Uh oh!

jeffersonrabb commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjellr commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffersonrabb commented Jan 21, 2019

Uh oh!

kjellr commented Jan 21, 2019

Uh oh!

jasmussen commented Jan 28, 2019

Uh oh!

afercia commented Feb 15, 2019

Uh oh!

aduth Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jeffersonrabb commented Jan 16, 2019 •

edited

Loading

jeffersonrabb Jan 18, 2019 •

edited

Loading

jasmussen commented Jan 18, 2019 •

edited

Loading

jasmussen commented Jan 21, 2019 •

edited

Loading

kjellr commented Jan 21, 2019 •

edited

Loading

aduth Feb 15, 2019 •

edited

Loading