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

Fix NaN bounds in FocalPointPicker #28406

Closed

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Jan 21, 2021

Two small changes here.

The main one is to address an issue (I've not seen reported) in which the focal point picker component causes a console error and ceases to allow adjustment in the horizontal axis. Perhaps this is an issue that may not occur on some setups but happens every time (EDIT: in Chrome) on my modestly powered laptop which is the only device I've tested on for now.

UPDATE: I've tested in Firefox and didn't reproduce (but didn't try very hard). It is possible according to this comment #28406 (comment). UPDATE № 2: I tested in Safari and found that I couldn't reproduce unless the console was opened 🙈

The second change is an unrelated minor documentation update. When searching for issues with this component I saw #16056 and figured it wouldn't hurt to address that too.

How has this been tested?

In a post with Cover or Media & Text blocks. Selecting such blocks and verifying there are no errors in the console. Using the Focal point picker and verifying that it works in both vertical and horizontal axes.

Screenshots

Before, with error and broken x-axis:

focal-point-picker-before.mp4

After, resolved:

focal-point-picker-after.mp4

Types of changes

Bug fix #28487
Documentation fix: #16056

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ajlende
Copy link
Contributor

ajlende commented Jan 21, 2021

I'm curious which browser you're using for the bug. I'm on Firefox 84.0.2 on MacOS 10.14.6.

The strange thing is that when I set a conditional breakpoint in the Firefox debugger to test this.mediaRef.current.clientWidth === 0, the breakpoint will trigger and then upon inspecting this.mediaRef.current.clientWidth while it's paused, the value is non-zero as it's supposed to be.

I've been able to reproduce it about 1 in 10 times when I click on the cover block fast enough after refreshing the page.

@stokesman
Copy link
Contributor Author

I've only tested on Chrome 87.0.4280.141 on macOS 10.12.6 and I don't have to hurry to click on the Cover or Media & Text block. I get the error every time.

That does seem strange with the debugger. I only used console.log in debugging this and definitely found that the 0 value was rapidly replaced with non-zero values. Just selecting the block causes multiple executions of calculateBounds in FocalPointPicker with only maybe two of the eight or so logs (don't remember exactly) having the zero value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover: unable to set Left value in the focal point picker Props incorrectly documented for FocalPointPicker
2 participants