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

Focal Point Picker #10925

Merged
merged 22 commits into from Jan 29, 2019

Conversation

@jeffersonrabb
Copy link
Contributor

jeffersonrabb commented Oct 22, 2018

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

This comment has been minimized.

Copy link
Contributor

mtias commented Oct 23, 2018

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).

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

@MichaelArestad

This comment has been minimized.

Copy link

MichaelArestad commented Dec 21, 2018

This is great!

@designsimply

This comment has been minimized.

Copy link
Contributor

designsimply commented Jan 16, 2019

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

@jeffersonrabb jeffersonrabb force-pushed the try/focal-point-picker branch from c1ecc3b to ded1017 Jan 16, 2019

@jeffersonrabb

This comment has been minimized.

Copy link
Contributor Author

jeffersonrabb commented Jan 16, 2019

Rebased and ready for review!

@jeffersonrabb jeffersonrabb requested review from WordPress/gutenberg-core and jasmussen Jan 16, 2019

@gziolo gziolo requested a review from karmatosed Jan 17, 2019

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

This comment has been minimized.

@gziolo

gziolo Jan 17, 2019

Member

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

This comment has been minimized.

@jeffersonrabb

jeffersonrabb Jan 18, 2019

Author Contributor

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:

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?

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

This comment has been minimized.

@gziolo

gziolo Jan 17, 2019

Member

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

This comment has been minimized.

@jeffersonrabb

jeffersonrabb Jan 18, 2019

Author Contributor

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?

This comment has been minimized.

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

This comment has been minimized.

@gziolo

gziolo Jan 17, 2019

Member

Only classnames is an external dependency here.

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

Everything else is internal dependencies.

This comment has been minimized.

@jeffersonrabb

jeffersonrabb Jan 18, 2019

Author Contributor

Resolved in 97fcd04

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 17, 2019

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 👍

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

This comment has been minimized.

@youknowriad

youknowriad Jan 18, 2019

Contributor

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

This comment has been minimized.

@jeffersonrabb

jeffersonrabb Jan 18, 2019

Author Contributor

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:
    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:
    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.

This comment has been minimized.

@jeffersonrabb

jeffersonrabb Jan 18, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 18, 2019

Really nice work.

GIF:

focalpoint

Can you make the focal point picker look like this?

group 2

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:

screenshot 2019-01-18 at 10 03 55

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

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 18, 2019

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

group 2

Display horizontal and vertical positions as percentages in text boxe…
…s, for accessibility reasons. Slight corrections to the postion calculations.
@jeffersonrabb

This comment has been minimized.

Copy link
Contributor Author

jeffersonrabb commented Jan 18, 2019

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?

@youknowriad
Copy link
Contributor

youknowriad left a comment

Nice work here, left some minor notes.

Show resolved Hide resolved packages/components/src/focal-point-picker/index.js Outdated
Show resolved Hide resolved packages/components/src/focal-point-picker/index.js Outdated
@kjellr

This comment has been minimized.

Copy link
Contributor

kjellr commented Jan 21, 2019

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:

ghost

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

@jeffersonrabb

This comment has been minimized.

Copy link
Contributor Author

jeffersonrabb commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

kjellr commented Jan 21, 2019

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.

@jeffersonrabb

This comment has been minimized.

Copy link
Contributor Author

jeffersonrabb commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

kjellr commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 28, 2019

🎉

@youknowriad youknowriad merged commit 77f6176 into master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Phase 2 design automation moved this from In progress to Done Jan 29, 2019

@youknowriad youknowriad deleted the try/focal-point-picker branch Jan 29, 2019

@karmatosed karmatosed added this to Widgets in Phase 2 Jan 31, 2019

@karmatosed karmatosed moved this from Widgets to Tighten Up in Phase 2 Jan 31, 2019

@kjellr kjellr moved this from Tighten Up to Done in Phase 2 Feb 4, 2019

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Feb 15, 2019

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. 🙂

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

This comment has been minimized.

@aduth

aduth Feb 15, 2019

Member

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.

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

This comment has been minimized.

@afercia

afercia Feb 15, 2019

Contributor

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

screenshot 2019-02-15 at 15 23 32

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.

This comment has been minimized.

@aduth

aduth Feb 15, 2019

Member

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?

This comment has been minimized.

@afercia

afercia Feb 15, 2019

Contributor

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?

This comment has been minimized.

@aduth

aduth Feb 15, 2019

Member

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.

image

This comment has been minimized.

@afercia

afercia Feb 15, 2019

Contributor

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.

@afercia afercia referenced this pull request Feb 15, 2019

Merged

Updates to the Font Size Picker #9802

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment