DioxusLabs / DioxusLabs/dioxus

A `style` update restores the declarations it dropped, so a style can never be removed

Open
#5,810 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.