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.

Sign up for GitHub

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

Jump to bottom

Event example does not work #241

Closed
ranok opened this issue Feb 26, 2023 · 8 comments
Closed

Event example does not work #241

ranok opened this issue Feb 26, 2023 · 8 comments

Comments

@ranok
Copy link

ranok commented Feb 26, 2023

Using the button.nim example from the README results in the same uncaught AssertionDefect as #86 and #123.

Below is an image of the DevTools console output when I click the button (I renamed the file to albiero, but otherwise it's a direct copy/paste from the examples.:
image

This is with karax 1.2.2 and nim 1.6.10.
@geotre
Copy link
Collaborator

geotre commented Feb 26, 2023

Recently fixed an issue (#232) where a browser extension was modifying the dom, seems like this could be similar. Once we can replicate the cause it should be possible to fix.

@ranok can you confirm which browser you are seeing this in, and with which extensions? If you have a lot of extensions installed, maybe try disabling some too see if you can figure out which is causing it.

You can also check the dom inspector in devtools to see if anything looks off (and post a screenshot here if so)

@ranok
Copy link
Author

ranok commented Feb 26, 2023

I'll try it in safemode and slowly add them back in shortly

@ranok
Copy link
Author

ranok commented Feb 26, 2023

Good catch, Adblock for Youtube oddly gets permissions for all sites and was somehow causing issues, by restricting it to only Youtube the example works again.

@ranok ranok closed this as completed Feb 26, 2023
@geotre
Copy link
Collaborator

geotre commented Feb 27, 2023

Thank you for confirming @ranok. I imagine the extension requests all pages so that it can also block ads on embedded youtube videos.

I would still like to fix this as it may impact other users. I can't replicate it by installing the extension, can you share what change it made to the browser dom?

@ranok
Copy link
Author

ranok commented Feb 27, 2023

Here are the two divs the extension injects:
image

If I make another div preceding the ROOT one that Karax uses, the extension injects into that one, and Karax works as expected:

<!doctype html>
<html>
    <head>
        <title>Testing</title>
    </head>
    <body id="body">
        <div id="preroot"></div><!-- the extension injects into this div now -->
        <div id="ROOT"></div>
        <script src="karax.js" type="text/javascript"></script>
    </body>
</html>

@geotre
Copy link
Collaborator

geotre commented Mar 1, 2023

Thank you @ranok. Could you re-open the issue too?

Here's a minimal reproduction:

import std/dom
include pkg/karax/prelude

var lines: seq[kstring] = @[]

proc createDom(): VNode =
  result = buildHtml(tdiv):
    button:
      text "Say hello!"
      proc onclick(ev: Event; n: VNode) =
        lines.add "Hello simulated universe"
    for x in lines:
      tdiv:
        text x

var modified = false

proc postrender =
  if modified: return
  modified = true
  document.getElementById("ROOT").appendChild(document.createElement("div"))

setRenderer(createDom, clientpostrenderCallback=postrender)

The error is triggered by this line. Compiling the example above with karax@#head using -d:release works as expected.

I'm not sure of the best approach to fix here. Developers may have browser extensions that do dom modifications (as we saw also in issue #232) so I don't think these checks should be run by default. Perhaps they could be opt in with a -d:karaxCheckSame flag or similar when needed for debugging? What do you think @Araq?

@ranok ranok reopened this Mar 1, 2023
@Araq
Copy link
Collaborator

Araq commented Mar 1, 2023

Perhaps they could be opt in with a -d:karaxCheckSame flag or similar when needed for debugging? What do you think @Araq?

Sounds good to me. At least I cannot think of a better solution.

@geotre geotre mentioned this issue Mar 2, 2023
Merged
@geotre
Copy link
Collaborator

geotre commented Mar 2, 2023

#244 has been merged, which should solve this issue. @ranok if you have a chance to check with your browser extension re-enabled please let us know if the issue persists.

I will close the issue again in the meantime.

@geotre geotre closed this as completed Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ranok @Araq @geotre