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

Forcibly call onblur on Inspector inputs before changing selection #240

Merged
merged 1 commit into from
Jul 8, 2020

Conversation

Rheeseyb
Copy link
Contributor

@Rheeseyb Rheeseyb commented Jul 7, 2020

Fixes #220

Problem:
Changes made to an Inspector input are not applied when clicking elsewhere in the editor

Fix:
(For the TLDR that doesn't require reading my life story, skip to the bit in bold at the bottom.)

I have been to the pits of hell with this one, and whilst this solution may be ugly, let me tell you this - it is the best you're getting without huge amounts of refactoring (even then don't count on it). This has been one of those issues where it seems tiny and easy to fix, so you make a fix and realise that something else is now getting in the way, but over and over and over. To give you an idea:

  1. React has a bug with it's onBlur handling when destroying components React onBlur events not firing during unmount facebook/react#12363 - this can be fixed by using a wrapper that ties the event handler directly to the DOM element instead, but that doesn't work because
  2. Inspector callbacks rely on a mutable ref to fetch the target elements when making a change, because we don't want element selection to trigger a re-render on every input if the value hasn't changed, so since the blur event is fired after selection changes the callback will try to apply the update to the wrong element. So we need some way to capture the context when focusing, right?
  3. Capturing the context when focusing requires a lot of messing around (using a ref to store the callback, changing that callback when another function is called, passing that other function down into the individual input fields), and on top of that we would have to manually ensure that every input is performing the correct dance to capture that context (believe me, I have tried so many different ways of achieving this in the Inspector, and the result was always either excessive rendering, or outdated contexts being used), Ok so let's try using the onClickOutside that we use...
  4. onClickOutside doesn't fire when clicking the canvas. U WOT M8?! No joke. It's because it works by adding an event listener to the document, but that event listener ends up being later in the queue than all of the others, meaning the selection has changed before it is reached, and it is removed upon destruction of the wrapped component. Ok fine, let's try adding a click catcher over the top of everything for forcibly blurring this shit...
  5. The portal is behind the editor (yep, no idea why it still works for the colour picker but it is). Ok fine, move that in front of the editor and use a clicker catcher on it when an input is focused...
  6. Using a clicker catcher means there is no way at all to then allow mouse events to bubble to elements that aren't direct descendants of the first element to handle that event. Totally forgot about this one because it caught us out a long while ago. That means you'd have to click twice to select something on the canvas (once to blur the input and remove the catcher, a second time to actually interact with the canvas). Bonus feature: gone are your cursors.

So, finally, in a case of "one last go before I throw in the towel", I found an existing case where we were tying a mousedown listener to the window, and made that check if the currently focused element was an inspector input (required to prevent it screwing up interaction with react-select elements and probably others), and the event was targeting a different element, in which case we forcibly call blur().

Commit Details:

  • Use a wrapper component for InspectorInput so that we could add the attribute data-inspector-input to it
  • Use an existing mousedown event listener attached to the window to check if a data-inspector-input should be blurred, and if so call blur() on it
  • Pass true for the capture param when adding the above listener to make sure it is handled before other listeners of that event (which should have been happening before, though I'm not sure if that old functionality is actually used anywhere)

Copy link
Contributor

@alecmolloy alecmolloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a truly cursed issue, thank you for diving into this rheese.

@Rheeseyb Rheeseyb merged commit edbb3ed into master Jul 8, 2020
@Rheeseyb Rheeseyb deleted the fix/inspector-input-onblur branch July 8, 2020 10:55
Copy link
Contributor

@seanparsons seanparsons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a solution.

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.

Clicking into the canvas doesn't fire onBlur for input elements in the inspector.
5 participants