Use setAttribute, not properties to fix SVG update

[MaxDesiatov]

Property assignment does not update SVG elements. We may want to use properties instead of setAttribute in the future for optimizations. For now I think that setAttribute works in all cases, including SVG, and seems safer to me.

Resolves #278.

[j-f1]

This causes a regression in the TextField demo — try typing in the first field then the second field on both main and this branch. Ultimately the solution imo is to have a list of keys that should be set as properties instead of attributes.

[MaxDesiatov]

I've introduced a new HTMLAttribute type to apply updates cleanly. There's no regression anymore, at least in the TextField example 🙂

[carson-katri]

Is the isUpdatedAsProperty a requirement for certain attributes or certain element & attribute combos?

[MaxDesiatov]

IDK, I wasn't able to find any information on this other than some people warning that setAttribute could be somehow slower that property assignment, but no appropriate benchmarks were present to verify this.

[j-f1]

I think it’s usually based on specific names. Here’s React’s file that defines how it handles attributes and properties: https://github.com/facebook/react/blob/92c7e49895032885cffaad77a69d71268dda762e/packages/react-dom/src/shared/DOMProperty.js

Here’s how it’s used for updating DOM nodes: https://github.com/facebook/react/blob/92c7e49895032885cffaad77a69d71268dda762e/packages/react-dom/src/client/DOMPropertyOperations.js#L129-L206

[MaxDesiatov]

Interesting that value is not in this list 🤔

// These are the few React props that we set as DOM properties
// rather than attributes. These are all booleans.
[
  'checked',
  // Note: `option.selected` is not updated if `select.multiple` is
  // disabled with `removeAttribute`. We have special logic for handling this.
  'multiple',
  'muted',
  'selected',

[carson-katri]

If its a set list, could we just automatically check them instead of each element specifying if it needs to be treated as a property?

[MaxDesiatov]

I thought about it, but attributes are created everywhere on every render cycle, which means that the check is multiplied by the number of times you use any attribute whatsoever. I'm not sure what's the best way to avoid the check on attribute creation other than hardcoding it as a property at the place of use.

[MaxDesiatov]

Another option would be to forbid attribute creation from a string literal and predefine all attributes in TokamakStaticHTML with an appropriate isUpdatedAsProperty value.

[carson-katri]

You could do it with type-safety and allow a string literal:

struct HTMLAttribute : ExpressibleByStringLiteral {
  ...
  static let value: Self = .init("value", isUpdatedAsProperty: true)
}

Something like that?

[MaxDesiatov]

Yeah, it wouldn't prevent anyone from still initializing value attribute from a string literal though. Probably still better than the current option...

[carson-katri]

Could make it more explicit with a .custom("myAttribute") initializer instead of a literal.

[MaxDesiatov]

Oh, requiring .custom is going to require changing hundreds of lines, I'd rather do that in a separate PR 😅

[j-f1]

it looks like there are a bunch of files in react-dom/src/client that handle form controls individually (ReactDOMInput.js, ReactDOMSelect.js, ReactDOMOption.js, ReactDOMTextarea.js) which seem to have the value handler inside of them.

[j-f1]

Either way I think this is a good start to unblock our user, and we can iterate more later.