Standardise vnode text representation #2670

barneycarroll · 2021-04-14T16:21:56Z

This addresses the crucial feature of #2669: text is always represented as virtual text nodes, never as a vnode.text. The vnode.text field remains in place for now.

The fix failed many tests which baked-in assumptions about vnode structure, so I took the opportunity of rewriting the tests to always use hyperscript expressions to avoid this category of brittle testing in future. In the process I removed a handful of tests which manually constructed vnode configurations which aren't feasible with hyperscript expressions, and fixed some text tests whose manual construction led them to validate impossible scenarios (boolean nodes are never treated as text nodes).

The fix as is creates a regression whereby <textarea> updates fail, which I'll need to fix.

The test cleanup creates its own strange bug in the <select> value update tests. I don't quite understand how this happened or what it's about, I'd appreciate a second pair of eyes.

barneycarroll · 2021-04-14T16:24:49Z

render/tests/test-attributes.js

-
-			o(succeeded).equals(false)
-		})
+	o.spec("contenteditable throws on untrusted children", function() {


The previous tests duplicated this sequence with a nebulous distinction between 'attr' & 'prop'

barneycarroll · 2021-04-14T16:50:34Z

render/tests/test-attributes.js

-				{tag:"option", attrs: {value: "0"}},
-				{tag:"option", attrs: {value: ""}}
-			]}
+			return m("select", attrs,


Constructing the select element itself via hyperscript rather than manually triggers these errors https://travis-ci.org/github/MithrilJS/mithril.js/builds/767047585#L293-L311:

attributes > select.value > updates with the same value do not re-set the attribute if the select has focus: 1 should equal 0 at /home/travis/build/MithrilJS/mithril.js/render/tests/test-attributes.js:639:35 attributes > select.value > updates with the same value do not re-set the attribute if the select has focus: 1 should equal 0 at /home/travis/build/MithrilJS/mithril.js/render/tests/test-attributes.js:644:35 attributes > select.value > updates with the same value do not re-set the attribute if the select has focus: 2 should equal 1 at /home/travis/build/MithrilJS/mithril.js/render/tests/test-attributes.js:649:35

Trying to isolate the issue from the DOM mocks, I can't replicate it with real DOM. Using a spy trap on the HTMLInputElement 'value' setter, both the manually constructed select and the hyperscript equivalent fail the first test but none of the others:

Flems

The spy was buggy

Well spotted! In this case, the DOM mock is correct, and the tests fail if we give them a hyperscript-generated select rather than a manually constructed one.

I'll have to dig into m.render and the mocks to try and figure out why the setter is triggered extraneously
(tomorrow)

In the hyperscript-generated vnode, the empty attrs object (i.e. {}) is re-set to null by execSelector(). This is the difference between the hyperscript-generated vnodes and the manually constructed ones in this case.

In execSelector(), instead of setting the attrs to null, it may be better to set an empty object or undefined.

@kfule brilliant! And fixing this would lead to further standardisation. Thanks so much!

barneycarroll · 2021-04-14T20:20:07Z

The <textarea> bug caused by this fix:

form inputs > textarea > updates after user input:
'bbb'
  should equal
'ccc'
    at /home/travis/build/MithrilJS/mithril.js/render/tests/test-input.js:293:29

Cannot be reproduced using real DOM

docs/migration-v02x.md

render/hyperscript.js

render/render.js

render/tests/test-createFragment.js

render/tests/test-createNodes.js

render/tests/test-onbeforeupdate.js

render/tests/test-oninit.js

gilbert · 2021-04-15T23:51:16Z

A worthy effort!

kfule · 2021-05-02T08:27:49Z

The <textarea> bug caused by this fix:
form inputs > textarea > updates after user input:
'bbb'
  should equal
'ccc'
    at /home/travis/build/MithrilJS/mithril.js/render/tests/test-input.js:293:29
Cannot be reproduced using real DOM

If you re-bundle the module, it will also fail the real DOM test.

If you move the value of the first child of vnode.children to vnode.attrs.value, as in the previous implementation using vnode.text, the test will pass for now.

barneycarroll · 2021-05-02T10:00:41Z

Thanks again @kfule !

So the rules of API precedence for textarea seem to be (lowest to highest):

defaultValue
Element subtree, if this contains text nodes with any character content
value

I therefore propose we rewrite these tests and follow Reacts lead in avoiding use of subtree to define textarea value. This should be present as a warning in the change log for now.

Neither 1 or 3 work as HTML values, but any DOM representation will be serialised with the element subtree, so this shouldn’t be a problem for SSR rendering (@StephanHoyer?).

Thanks again for your insights @kfule

StephanHoyer · 2022-01-26T08:14:02Z

@barneycarroll I would really like to see this in the next release. Would be great if you can rebase and cleanup this PR so I can review and test this.

barneycarroll · 2022-01-26T18:56:37Z

Sure, I’ll have a look tomorrow.

StephanHoyer · 2022-02-09T19:40:13Z

@barneycarroll Are you still interested in doing this? If not it's ok, then I can take a look at it.

StephanHoyer · 2022-02-12T10:27:19Z

@barneycarroll just rebased without issues

StephanHoyer · 2022-02-12T11:19:53Z

@barneycarroll Merged!

barneycarroll requested a review from pygy April 14, 2021 16:21

barneycarroll requested a review from dead-claudia as a code owner April 14, 2021 16:21

barneycarroll commented Apr 14, 2021

View reviewed changes

dead-claudia requested changes Apr 15, 2021

View reviewed changes

docs/migration-v02x.md Outdated Show resolved Hide resolved

render/hyperscript.js Outdated Show resolved Hide resolved

render/render.js Show resolved Hide resolved

StephanHoyer reviewed Apr 15, 2021

View reviewed changes

render/tests/test-createFragment.js Outdated Show resolved Hide resolved

StephanHoyer reviewed Apr 15, 2021

View reviewed changes

render/tests/test-createNodes.js Outdated Show resolved Hide resolved

StephanHoyer reviewed Apr 15, 2021

View reviewed changes

render/tests/test-onbeforeupdate.js Outdated Show resolved Hide resolved

StephanHoyer reviewed Apr 15, 2021

View reviewed changes

render/tests/test-oninit.js Outdated Show resolved Hide resolved

barneycarroll mentioned this pull request Apr 15, 2021

onchange event handler breaks select with selected option #2107

Closed

barneycarroll force-pushed the standardise-vnode-text branch from 429cc4a to 75c6005 Compare May 2, 2021 14:23

barneycarroll added 11 commits February 12, 2022 11:26

Tests use hyperscript instead of manually constructing nodes

4a4abcb

Tests use hyperscript instead of manually constructing nodes

5b26b92

Text is always represented as text vnodes, never as vnode.text

fa0ca91

Fix lint errors

a0e869a

Back out migration ToC, correct references to vnode shape

7000c03

Remove redundant child normalisation

65f0d4b

Non-mutating m.fragment/m.trust references

1442eac

Back out migration ToC, correct references to vnode shape

fb5dcd0

Back out migration ToC, correct references to vnode shape

aac2813

Updating textarea subtree should not be expected to override user input

c9a16c2

vnode.attrs is never null

40a2380

Reverting erroneous docs formatting

d223902

StephanHoyer force-pushed the standardise-vnode-text branch from c0fd201 to d223902 Compare February 12, 2022 10:26

StephanHoyer added 2 commits February 12, 2022 12:02

Fix ospec timeout usage

9a82acc

Add notice to changelog

dad8b6d

StephanHoyer merged commit e4bd203 into next Feb 12, 2022

StephanHoyer deleted the standardise-vnode-text branch February 12, 2022 11:19

JAForbes mentioned this pull request Apr 28, 2022

Release v2.1.0 #2766

Merged

orbitbot added the patch label May 1, 2022

chadlwilson mentioned this pull request May 23, 2022

ui: bump mithril and @types/mithril in /server/src/main/webapp/WEB-INF/rails gocd/gocd#10444

Merged

SychO9 mentioned this pull request May 31, 2023

chore: upgrade to mithril 2.2 flarum/framework#3831

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardise vnode text representation #2670

Standardise vnode text representation #2670

barneycarroll commented Apr 14, 2021

barneycarroll Apr 14, 2021

barneycarroll Apr 14, 2021

barneycarroll Apr 14, 2021 •

edited

Loading

pygy Apr 15, 2021

barneycarroll Apr 15, 2021

pygy Apr 15, 2021

kfule May 2, 2021

barneycarroll May 2, 2021

barneycarroll commented Apr 14, 2021

gilbert commented Apr 15, 2021

kfule commented May 2, 2021

barneycarroll commented May 2, 2021 •

edited

Loading

StephanHoyer commented Jan 26, 2022

barneycarroll commented Jan 26, 2022

StephanHoyer commented Feb 9, 2022

StephanHoyer commented Feb 12, 2022

StephanHoyer commented Feb 12, 2022

Standardise vnode text representation #2670

Standardise vnode text representation #2670

Conversation

barneycarroll commented Apr 14, 2021

barneycarroll Apr 14, 2021

Choose a reason for hiding this comment

barneycarroll Apr 14, 2021

Choose a reason for hiding this comment

barneycarroll Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

pygy Apr 15, 2021

Choose a reason for hiding this comment

barneycarroll Apr 15, 2021

Choose a reason for hiding this comment

pygy Apr 15, 2021

Choose a reason for hiding this comment

kfule May 2, 2021

Choose a reason for hiding this comment

barneycarroll May 2, 2021

Choose a reason for hiding this comment

barneycarroll commented Apr 14, 2021

gilbert commented Apr 15, 2021

kfule commented May 2, 2021

barneycarroll commented May 2, 2021 • edited Loading

StephanHoyer commented Jan 26, 2022

barneycarroll commented Jan 26, 2022

StephanHoyer commented Feb 9, 2022

StephanHoyer commented Feb 12, 2022

StephanHoyer commented Feb 12, 2022

barneycarroll Apr 14, 2021 •

edited

Loading

barneycarroll commented May 2, 2021 •

edited

Loading