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.

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

Cover: unable to set Left value in the focal point picker #28487

Closed
flootr opened this issue Jan 26, 2021 · 7 comments · Fixed by #28499
Closed

Cover: unable to set Left value in the focal point picker #28487

flootr opened this issue Jan 26, 2021 · 7 comments · Fixed by #28499
Assignees
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Milestone

Comments

@flootr
Copy link
Contributor

flootr commented Jan 26, 2021

Description

Since 9.8.0 for the Cover block I'm unable to set the Left value in the focal point picker.

Step-by-step reproduction instructions

  1. In Gutenberg add a new Cover block and set an image
  2. Click on the block to access detailed settings
  3. Try to set a Left value in the focal point picker

Expected behaviour

It'd set the Left value accordingly.

Actual behaviour

It doesn't set the Left value at all.

Screenshots or screen recording (optional)

focal-point-picker.mp4

WordPress information

  • Gutenberg: v9.8.1
  • WordPress: 5.6

All other plugins except for Gutenberg have been deactivated.

Device information

  • Device: Desktop
  • Operating system: macOS Big Sur 11.1
  • Browser: Chrome 88

/cc @sirreal

@sirreal sirreal added [Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended labels Jan 26, 2021
@sirreal sirreal added this to the Gutenberg 9.9 milestone Jan 26, 2021
@sirreal sirreal added the [Type] Regression Related to a regression in the latest release label Jan 26, 2021
@flootr
Copy link
Contributor Author

flootr commented Jan 26, 2021

Addition: I noticed that it's possible to adjust the Left value after I saved the post for the first time and then reload the page.

@mtias mtias added [Block] Cover Affects the Cover Block - used to display content laid over a background image and removed [Block] Image Affects the Image Block labels Jan 26, 2021
@mtias
Copy link
Member

mtias commented Jan 26, 2021

There was a recent fix around this: #28350

@ockham
Copy link
Contributor

ockham commented Jan 26, 2021

There was a recent fix around this: #28350

I cherry-picked that to the 9.8 branch tho, so it should be included in the release this was tested with 🤔 Could be related to one of the other recent Cover block fixes/adjustments? cc /@jasmussen

@brentswisher
Copy link
Contributor

I believe this was caused in #28096 because updateBounds was added to componentDidMount, it runs after the mediaRef exists, but before it has received the width/height of the image?

This results in a division by 0 which errors out and sets the left of the focal point picker to be NaN and causes it to stop functioning. I added a check to the focalPointPicker in #28499 with should catch this and prevent the error if someone wants to test and review it, I think it might just be able to make it into Gutenberg 9.9 but am not sure on the timing.

Pinging @ItsJonQ since he might see a better way to handle this?

@stokesman
Copy link
Contributor

stokesman commented Jan 26, 2021

There was a recent fix around this: #28350

That does appear to have been a separate issue. It's likely Brent has identified the cause for this in the above comment. I'd already made #28406 to address this (it didn't have any issues I could find at the time). The code differs trivially but does the same thing as #28499 and I threw in a doc fix 🤷‍♂️

@ockham
Copy link
Contributor

ockham commented Jan 27, 2021

Since the Focal Point Picker has been a bit fragile lately, it'd be great if we could add some e2e test coverage to the Cover Block e2e test (possibly alongside a fix, such as #28499). Anyone on this issue who'd like to give this a shot? 🙏

@mtias
Copy link
Member

mtias commented Jan 27, 2021

The performance of dragging also seems a bit clunky to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
6 participants