georgealways / lil-gui

Makes a floating panel for controllers on the web. Works as a drop-in replacement for dat.gui in most projects.
https://lil-gui.georgealways.com/
MIT License
1.13k stars 43 forks source link

Folder close or open eats keystrokes #69

Closed jxtplatinum closed 1 year ago

jxtplatinum commented 2 years ago

I have a super simple lil-gui demo that has one folder with one item. I also have a window event listener for 'keydown' events because I eventually want to enable shortcut keys for various behaviors. Upon startup, the folder is open and the event listener works as expected. But once the folder is closed, or even if reopened, keydown events never make it to my event listener until I click outside the lil-gui window. (This behavior happens with both Safari and Firefox on a Mac running macOS Monterey 12.4.)

Seems like a bug to me. Can this behavior be fixed? Thanks.

georgealways commented 2 years ago

Hi there! Thanks for all the detail, this is a good catch. That happens because of this.

The point is to make it so that typing in a controller doesn't trigger global key commands: imagine I have an app with WASD controls and I want to type the letter W in a lil-gui textbox.

I wouldn't have expected clicking a folder to trigger this, but I think I know why it does: folder buttons are keyboard focusable via tabindex. Since they can be focused, they too will capture key events and stop them from propagating.

Right now I'm stopping all key events from propagating at the GUI's root DOM element for brevity, but maybe we go back to a solution like #22 where it's applied to individual input elements.

Short-term, I would recommend rebuilding the library without lines 181 & 182 in GUI.js, but I'll keep thinking about a proper solution.

jxtplatinum commented 2 years ago

Thanks for the quick response. I don't really know how to "rebuild the library", but I did find and comment out those lines in my copy of lil-gui.esm.js, which fixed my problem.

ppizarror commented 1 year ago

Hi! I faced the same problem today and reached the same solution.

Could be a solution to check if the GUI controller has a focus (this.domElement === document.activeElement, and compare to their child recursively...)?. Then:

...
this.domElement.addEventListener( 'keydown', (e) => {
   if (this.hasFocus()) e.stopPropagation();
} );
...
dipterix commented 1 year ago

Leaving marks... Same problem here.

I suggest one way to fix is to make event handlers as class methods using fat arrows. Users can remove the event listeners if they want, or re-link (put theirs ahead)...


class ... {
  _onKeyDown = ( e ) => { e.stopPropagation(); }

  constructor(...) {
    this.domElement.addEventListener( 'keydown', this._onKeyDown );
  }

  destroy () {
    this.domElement.removeEventListener( 'keydown', this._onKeyDown );
    super.destroy();
  }
}

Unfortunately, I tried on a fork, rollup seems not like fat arrows in class method ?

georgealways commented 1 year ago

Hi all—my current plan for this is to bind the stopPropagation key events to each Controller's domElement as opposed to the entire GUI domElement. This way the behavior would only apply to controllers and not folder headers. Curious to hear your reactions to that approach.

Edit, tagging on some other ideas:

dipterix commented 1 year ago

Hi @georgealways personally I think this is a better idea (bind key events to each Controller's domElement). My concern is should e.stopPropagation() get called only when input dom are focused? If they are not focused, that is not considered "typing"?

Also for Issue https://github.com/georgealways/lil-gui/issues/83 , it asked whether scroll can be disabled, I guess you can also make it "unscrollable" only when texts are focused or mouse enters the $sliders... Then there shouldn't be such complaints (maybe)

georgealways commented 1 year ago

Thanks for the input @dipterix, I was thinking along the same lines. Let's please keep discussion of the other issue to its own thread.

georgealways commented 1 year ago

@jxtplatinum @ppizarror As I have it implemented now, Space and Enter will still be "eaten" by folders since they're used to toggle open/closed. Wanted to get a reaction to that from interested parties.

jxtplatinum commented 1 year ago

Thanks. That does not impact my current usage because I don't use space or enter for anything. Seems like a reasonable compromise. However, I might suggest having an option to turn off the space/enter folder shortcuts in case a developer wishes to use space or enter for his or her own purposes.

georgealways commented 1 year ago

This is fixed as of 0.18.2—thanks for being patient :)

dipterix commented 1 year ago

This is fixed as of 0.18.2—thanks for being patient :)

Can't wait to try

georgealways commented 4 months ago

Hi all, I'm going to be adding a flag that disables the stopPropagation() code. If you set this flag to false, page-level keydown/up listeners will still trigger when typing in Controllers. Users will have to manually determine from where these events originated.

I'm deciding between two names, and I wanted to get a sense of what people find more intuitive:

new GUI( { captureKeys: false } ) new GUI( { stopPropagation: false } )

cc @dboddy @AlaricBaraou

dboddy commented 4 months ago

Hi, sounds good. I would suggest: new GUI( { allowPropagation: true } ) as I think double negatives are not as clear.

AlaricBaraou commented 4 months ago

I prefer {{ captureKeys: false }} personally. It's very explicit about what it does I think. But both are fine in my opinion.