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

Problem integrating in nw.js app #39

Closed
SkyBladeCloud opened this issue Jan 30, 2022 · 8 comments
Closed

Problem integrating in nw.js app #39

SkyBladeCloud opened this issue Jan 30, 2022 · 8 comments

Comments

@SkyBladeCloud
Copy link

SkyBladeCloud commented Jan 30, 2022

Nw.js is a development platform by Intel which allows application programmers to combine a nodejs backend with a HTML/CSS/JS frontend into a standalone app (similar to Electron):

https://nwjs.io/

In Nw.js, visible HTML interfaces don't usually use the default, global "window". Rather, the global window corresponds to an invisible, background object. This is an useful design choice for headless environments. However, it has 3 problems with lil-gui:

1 - By default, lil-gui will attempt to add itself to the global (invisible) document.
2 - By delfault, styles will be injected in the head of the global invisible document.
3 - Event listeners will be registered in the global invisible document as well.

The first couple of issues can be worked around by using lil-gui's already available constructor parameters:

1 - Setting a custom HTML node from a visible document in the Nw.js app works and lil-gui's frame shows up.
2 - Disabling default style injection and setting a custom CSS in the visible document works as well.

While these couple of workarounds show a perfectly visible lil-gui frame on my Nw.js app, it's non-functional due to the fact that its event listeners are still being registered in the default, global document.

Thus, I'd like to ask: would it be possible to provide an optional, custom window/document for lil-gui to use (for instance, as a constructor parameter), instead of the global one?

Thank you very much and kind regards,

~Sky

@georgealways
Copy link
Owner

Hi Sky, that all makes sense. I'm guessing that specifically it's the references to window and document.body in NumberController? I'm pretty sure that's the only place those appear outside of GUI's constructor (which it sounds like you're avoiding with the container parameter).

Let me know if you can share any stack traces or line numbers, it would be nice if lil-gui worked in an environment like this.

@jonlepage
Copy link

jonlepage commented Feb 1, 2022

I get no issue to integrate in nwjs on my side, but...
Am confused because i cant see listeners in events registers from the dev tool in the SDK !
I can see all my custom listeners, and other libs listeners but not lil-gui listeners !
Probably related but need more test to understand why, i didn't study yet the low level of the lib maybe if i have time one day!
image


Also i think it should a good idea to make listeners to passive as chromium warn !
image
and it seem not a micro-optimisation related to this status and can have high impact on cpu.
https://www.chromestatus.com/feature/5745543795965952


edit: i cant give you the direct line of code because my bundler Vitejs build and use a ems version, but here the part of code seem make chromium angry .
image

Am also joint Skyblade about points he exposed , i get some styles issue with the current css pattern injection where i can see sometime some crushing, so i add some patch in the flow to avoid those issue.
In my case, root collide with my root css, and maybe more soon , we will see.
I need remove the class tag like this.

		useEffect( ()=>{
			if( $Inspect.current && ref.current ){
				const entity = $Inspect.current;
				const metagui = new GUI( { autoPlace:false, container:ref.current, title:entity.uid } );
				metagui.domElement.classList.remove( 'root' );// Root bug with vudge root

This is called monkey pathing see and we should avoid 😅
But i feel your pain, i think we all hate do the CSS part and is hellish to make a good structure for a public share.
You will may and probably need to renames with less commun words or add more complexe alias for public usage or found a pro frontend dev who love code in css.
Or maybe more safe way for directly inject css in js and provide a method or a api for user customisation.
All solution will for sure be a break change ! but for me is fine, i will probably just do monkeyPatching in a custom css file instead in the flow and i will not die.

gl 😋

@georgealways
Copy link
Owner

georgealways commented Feb 1, 2022

@djmisterjon

I can see all my custom listeners, and other libs listeners but not lil-gui listeners !

I wonder if that's the same issue the original post is describing?

Also i think it should a good idea to make listeners to passive as chromium warn.

I'm not able to reproduce these warnings in Chrome or Chromium. Is this specific to nw.js?

In my case, root collide with my root css, and maybe more soon , we will see.

I'd like to keep the class names intuitive, for now I'd recommend that you make your .root selector more specific so it isn't applied to the panel. Let's keep discussion about this in #29.

@jonlepage
Copy link

jonlepage commented Feb 1, 2022

@djmisterjon

Also i think it should a good idea to make listeners to passive as chromium warn.

I'm not able to reproduce these warnings in Chrome or Chromium. Is this specific to nw.js?

no, my theory is i think its related to when its injected inside a virtual dom like react.
i didn't test in vuejs or other frame work, but its should send same warn.
https://codesandbox.io/s/musing-fermat-o1ecm

image
Also feel free to share and add the react template demo in doc for people who curious to know how to use with.

@georgealways
Copy link
Owner

@djmisterjon Yea, that's strange, I'm still not able to get those warnings to show up from your link. I actually can't make those event listeners passive because they need to call preventDefault—you can safely ignore those warnings.

@georgealways
Copy link
Owner

@djmisterjon I managed to locate those warnings in the console and I have a fix ready in the next release. I also didn't realize that you can specify { passive: false } to stop the warnings while still being able to call preventDefault. 🎉

@SkyBladeCloud I'd still like to figure out your original issue as well!

I'm guessing that specifically it's the references to window and document.body in NumberController? I'm pretty sure that's the only place those appear outside of GUI's constructor (which it sounds like you're avoiding with the container parameter).

Let me know if you can share any stack traces or line numbers, it would be nice if lil-gui worked in an environment like this.

@SkyBladeCloud
Copy link
Author

SkyBladeCloud commented Mar 6, 2022

Hello all,

Thank you very much for your support and sorry for the late answer.

I do believe those "addEventListener" calls within NumberController are the cause of my issue. I don't have the means to build the library to test myself (yet), but as mentioned in my original post, "window" is a completely different entity in the context of a NW.js application...

...At least by default. I've found that NW.js does provide a feature to "merge" the default backend "window" with the frontend one, using the so-called "mixed context mode"

https://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#mixed-context-mode

In this mode, "window" is what lil-gui expect and the library works flawlessly for my use cases. However, mixed context has some drawbacks and for now I'm considering it a workaround in my application. I wonder if this is how @djmisterjon got the lib working on his environment, or if he uses another way I'm not aware of.

I'll do some further test, see if I can build the library and provide further insight.

EDIT: Some insight that might be useful:

1 - There are 2 calls to requestAnimationFrame in GUI.js. These will never be fired in NW.js, given the background nature of the default window (in separate context mode). These calls were the main reason why the library was not working in my case, changing them to request the animation frame on the foreground window (accessible as nw.Window.get().window) makes the folders open and close as expected.

2 - Even after this, sliders wouldn't work as expected due to their event listener registered on the background window. As you had predicted, changing NumberController (in my use cases, at lines 257, 258, 384, 385, 268 and 269) in order to reference the right window/document fixes all issues for me! :)

There are of course other uses of window/document throughout the library. Would it make sense to have a window object as optional constructor parameter to use instead of the default global one (falling back to it, if the parameter is not provided)?

Actually it could be even better to have lil-gui detect the window/document of the DOM Element passed in the constructor, like so:

https://stackoverflow.com/a/16010322

Thanks again and kind regards,

~Sky

@georgealways
Copy link
Owner

Thanks for all the extra detail Sky. Ultimately though, I think this is the sort of thing that should stay in your fork. Abstracting every reference to window or document will make development harder going forward. I think the library should operate under the assumption that basic browser things like document or requestAnimationFrame exist and work.

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

No branches or pull requests

3 participants