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: Color picker rendering flaw #1668

Merged
merged 1 commit into from Jun 26, 2023
Merged

Conversation

ptomask
Copy link
Contributor

@ptomask ptomask commented Jun 20, 2023

This was reported as the ColorPicker was unable to be edited by any input fields in grid view but works correctly in form view.

Further investigations revealed that the field is actually broken in both grid and form views and the main issue is an infinite loop of ColorPicker wasted renders. The exact mechanism of why that only caused the editing issue in grid views and not form views remains unknown, expectation is rather being not properly editable in both, I suspect the difference in details of how the focusing works in grid and form views, which caused any internal color picker input field getting the focus stolen each render in grid view and not in form view.

The infinite loop was present because of an unstable function reference passed to div element. Rough loop description follows:

  1. The div element receives new ref reference
  2. So the reference gets called.
  3. Dropdown's Measure component measured child ref gets called by reference function from previous point.
  4. Measure component rerenders its measured content, because its dimensions might have changed.
  5. The content contains the div mentioned in pt. 1 closing the loop.

Conclusion points:

  1. We shall investigate React rendering performance from time to time. Here "Highlight updates" from React DevTools helped a lot.
  2. We shall think twice before directly passing an arrow function to a component prop.
  3. Maybe this would not happen if the Measure component is little bit more well-behaved, but here the "think twice about unstable references" point is of much stronger importance, I would not blame the Measure component.

@origambot
Copy link

This pull request has been mentioned on ORIGAM Community. There might be relevant details there:

https://community.origam.com/t/color-grid-editor-it-is-not-possible-to-enter-value-into-hex-field-or-rgb-fields/2633/3

@washibana washibana merged commit 2c03ae1 into master Jun 26, 2023
10 checks passed
@washibana washibana deleted the fix-color-picker-rendering branch June 26, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants