DioxusLabs / DioxusLabs/dioxus
A `style` update restores the declarations it dropped, so a style can never be removed
- Dominant language
- Rust
- Stars
- 39.1k
- Forks
- 1.9k
- Avg merge
- 4d 10h
- Merged PRs (30d)
- 4
Description
## Environment
- Dioxus `0.8.0-alpha.1` (`e2cc82e63`), `dioxus-web`, `rustc 1.97.1`,
Chromium 1200 (Playwright). The bug is in the shared JS interpreter, so it
is browser-independent; the code is byte-identical in upstream `main`.
## Problem
Changing a declaration in a `style` attribute works; *dropping* one does
not. `style: "color: red; background: blue"` re-rendered as
`style: "color: red"` leaves the element blue, its DOM attribute unchanged.
Custom properties are the sharp edge: a rule `background: var(--x, white)`
can never fall back to `white` once the element has set `--x`.
## Root cause
`packages/interpreter/src/ts/set_attribute.ts`, `case "style"`:
```ts
const existingStyles: Record = {};
for (let i = 0; i < node.style.length; i++) {
const prop = node.style[i];
existingStyles[prop] = node.style.getPropertyValue(prop);
}
node.setAttribute(field, value); // correct: replaces the block
for (const prop in existingStyles) { // then puts back everything
if (!node.style.getPropertyValue(prop)) // the new value did not set
node.style.setProperty(prop, existingStyles[prop]);
}
```
The restore exists to protect declarations the attribute does not own -
properties written through the `style` *namespace* (`background: "red"` in
rsx, handled at the top of the same function by `node.style.setProperty`),
which a whole-attribute write would wipe. The snapshot cannot tell those
from the attribute's own declarations, so it protects both - and the
attribute's are exactly the ones that must go.
Removal proper is unaffected: dropping the attribute entirely goes through
`removeTopAttribute` -> `removeAttribute("style")`. Only a *shortened* value
is silently undone.
## Minimal reproduction
```rust
fn app() -> Element {
let mut wide = use_signal(|| true);
rsx! {
button { id: "toggle", onclick: move |_| wide.toggle(), "toggle" }
div {
id: "target",
style: if wide() { "color: red; background: blue" } else { "color: red" },
"hi"
}
}
}
```
Clicking the button should drop the blue background. Measured in Chromium on
a `dx build --platform web` of exactly this app:
```
before click: attr="color: red; background: blue" background=rgb(0, 0, 255)
after click: attr="color: red; background: blue;" background=rgb(0, 0, 255)
```
`dioxus-core` emits the right mutation for that render -
`SetAttribute { name: "style", value: Text("color: red") }`, recorded
through a `WriteMutations` impl over a `VirtualDom` - so the fault is
entirely in how the interpreter applies it.
## Patch
Track the property names the **attribute** last declared on the node, and
let only those go; everything else in the block is foreign and is restored,
as today.
```diff
--- a/packages/interpreter/src/ts/set_attribute.ts
+++ b/packages/interpreter/src/ts/set_attribute.ts
@@
+// The property names the `style` attribute last declared on a node.
+// Anything else in the block is foreign - a `style`-namespace write
+// (`background: "red"` in rsx), or a declaration inherited from the
+// template this node was cloned from - and must survive an attribute
+// write. Attribute-owned declarations must not.
+const attributeStyles = new WeakMap>();
+const styleScratch = document.createElement("div");
+
+function attributeStyleNames(css: string): Set {
+ styleScratch.style.cssText = css || "";
+ const names = new Set();
+ for (let i = 0; i < styleScratch.style.length; i++) {
+ names.add(styleScratch.style[i]);
+ }
+ return names;
+}
+
export function setAttributeInner(node, field, value, ns) {
@@
- case "style":
- // Save the existing styles
- const existingStyles: Record = {};
- for (let i = 0; i < node.style.length; i++) {
- const prop = node.style[i];
- existingStyles[prop] = node.style.getPropertyValue(prop);
- }
- // Override all styles
- node.setAttribute(field, value);
- // Restore the old styles
- for (const prop in existingStyles) {
- if (!node.style.getPropertyValue(prop)) {
- node.style.setProperty(prop, existingStyles[prop]);
- }
- }
- break;
+ case "style": {
+ // Only declarations this attribute did not write last time are saved;
+ // the first write on a node treats the whole block as foreign, which
+ // is the old behaviour.
+ const previous = attributeStyles.get(node);
+ const foreign: Record = {};
+ for (let i = 0; i < node.style.length; i++) {
+ const prop = node.style[i];
+ if (previous === undefined || !previous.has(prop)) {
+ foreign[prop] = node.style.getPropertyValue(prop);
+ }
+ }
+ node.setAttribute(field, value);
+ for (const prop in foreign) {
+ if (!node.style.getPropertyValue(prop)) {
+ node.style.setProperty(prop, foreign[prop]);
+ }
+ }
+ attributeStyles.set(node, attributeStyleNames(value));
+ break;
+ }
```
`src/js/set_attribute.js` is generated (`bun build` via
`packages/interpreter/build.rs`) and must be regenerated.
**Rejected alternative:** recording the property names written through the
`style` namespace in a `WeakMap` and restoring only those. It looks
equivalent and is simpler, but it fails: a namespaced write lands on the
*template* node, which is then cloned per instance, so the live element is
never in the map and its template-applied declarations are dropped. Measured
- it breaks the `merge styles` case below.
## Verification
Chromium (Playwright), against a `dx build --platform web` of the
reproduction app, with the interpreter's own generated
`set_attribute.js`/`inline0.js` patched in the built output:
| | attribute after click | computed background |
|---|---|---|
| unpatched | `color: red; background: blue;` | `rgb(0, 0, 255)` |
| patched | `color: red` | `rgba(0, 0, 0, 0)` |
Toggling back and forth round-trips correctly (blue -> none -> blue -> none).
No regression on the case the restore exists for. The app also renders
upstream's `merge styles` fixture from `packages/playwright-tests/web`:
```rust
div { id: "merge-styles-div", style: "width: {px}px; height: {px}px", background_color: "red" }
```
which keeps `rgb(255, 0, 0) 100px 100px` before and after the click, patched
and unpatched alike - i.e. the namespaced `background-color` still survives
the whole-attribute write. The call order was confirmed by instrumenting
`setAttributeInner`: the namespaced write happens first (on the template),
the attribute write second (on the clone).
**Not run:** the full `packages/playwright-tests` suite.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with packages/interpreter/src/ts/set_attribute.ts and its style case, then inspect packages/interpreter/build.rs and the generated src/js/set_attribute.js. Reproduce the toggle with the provided Dioxus app and check the merge-styles fixture in packages/playwright-tests/web. Done means shortened style attributes remove dropped declarations while namespaced styles survive, with generated output regenerated.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- css, playwright, rust, typescript
- Domain
- frontend, testing
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100