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

egui_text_agent is not removed from the dom when WebRunner.destroy() is called. #3618

Closed
LiamPClancy opened this issue Nov 24, 2023 · 1 comment · Fixed by #4561
Closed
Labels
bug Something is broken web Related to running Egui on the web

Comments

@LiamPClancy
Copy link

LiamPClancy commented Nov 24, 2023

Describe the bug
I'm loading a WASM package into a a react app, it's using egui to render the plot and rust to do logic.
when calling wasmHandle.start(...)
it adds the text agent with
super::text_agent::install_text_agent(self)?;

but when calling
wasmHandle.destroy()
the text_agent is not removed from the dom,
This is creating an issue if the wasmHandle.start(...) is called again.

my use case is that when someone logs out the wasmHandle is destroyed, and if they log back in again then .start is called.
i'm handling this by removing the egui_text_agent when the user logs out, but it'd be better i think if it was part of the destroy method.

I'm thinking to add

pub fn remove_text_agent() -> Result<(), JsValue> {
    if let Some(element) = web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .get_element_by_id(AGENT_ID)
    {
        if let Some(parent) = element.patent_node() {
            patent.remove_child(&element)?;
        }
    };

    Ok(());
}

to the text_agent.rs

and then modifying webRunner destroy with a call to remove_text_agent()

pub fn destroy(&self) {
        self.unsubscribe_from_all_events();
        super::text_agent::remove_text_agent()?;
        if let Some(runner) = self.runner.replace(None) {
            runner.destroy();
        }
    }

Happy to make a PR if you think the above is the correct approach?

@LiamPClancy LiamPClancy added the bug Something is broken label Nov 24, 2023
@emilk emilk added the web Related to running Egui on the web label Nov 24, 2023
@emilk
Copy link
Owner

emilk commented Nov 24, 2023

Sounds good - please make a PR!

@jprochazk jprochazk mentioned this issue May 28, 2024
5 tasks
hacknus pushed a commit to hacknus/egui that referenced this issue Oct 30, 2024
- Closes emilk#4060 - no longer aligned
to top
- Closes emilk#4479 - `canvas.style` is
not set anywhere anymore
- Closes emilk#2231 - same as emilk#4060
- Closes emilk#3618 - there is now one
`<input>` per `eframe` app, and it's removed transitively by
`WebRunner::destroy -> AppRunner::drop -> TextAgent::drop`

This PR improves the text agent to make fewer assumptions about how
`egui` is embedded into the page:
- Text agent no longer sets the canvas position
- There is now a text agent for each instance of `WebRunner`
- The input element is now moved to the correct position, so the OS can
display the IME window in the correct place. Before it would typically
be outside of the viewport

The best way to test this is to build & server the web demo locally:
```
scripts/build_demo_web.sh && scripts/start_server.sh
```

Then open the EasyMark editor, and try using IME to input some emojis:
http://localhost:8888/#EasyMarkEditor

To open the emoji keyboard use:
- <kbd>win + .</kbd> on Windows
- <kbd>ctrl + cmd + space</kbd> on Mac

Tested on:
- [x] Windows
- [x] Linux
- [x] MacOS
- [x] Android
- [x] iOS

## Migration guide

The canvas no longer controls its own size/position on the page. This
means that those properties can now be controlled entirely via HTML and
CSS, and multiple separate `eframe` apps can coexist better on a single
page.

To match the old behavior, set the `canvas` width and height to 100% of
the `body` element:

```html
<html>
  <body>
    <canvas></canvas>
  </body>
</html>
```

```css
/* remove default margins and use full viewport */
html, body {
  margin: 0;
  width: 100%;
  height: 100%;
}

canvas {
  /* match parent element size */
  width: 100%;
  height: 100%;
}
```

Note that there is no need to set `position: absolute`/`left: 50%;
transform: translateX(-50%)`/etc., and setting those properties may
poorly affect the sharpness of `egui`-rendered text.

Because `eframe` no longer updates the canvas style in any way, it also
means that on mobile, the canvas no longer collapses upwards to make
space for a mobile keyboard. This should be solved in other ways:
emilk#4572
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken web Related to running Egui on the web
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants