Use “moveBefore” for map shuffling. #343
Conversation
| and `disconnectedCallback` are no longer called because elements are only | ||
| moved in the DOM rather than being disconnected and reconnected. Added test | ||
| coverage for `connectedMoveCallback` and reference tracking during element | ||
| reordering (#254). |
There was a problem hiding this comment.
In case this issue doesn’t ring a bell — we specifically have an outstanding bug related to shuffling tabs for an editor which can currently cause monaco to be disposed and then recreated if moving tabs around!
| } | ||
|
|
||
| connectedMoveCallback() { | ||
| super.connectedMoveCallback(); |
There was a problem hiding this comment.
This is the only line in our test suite that exercises this callback — just for coverage.
| assert(container.querySelector('#target').children[2].classList.contains('true')); | ||
| }); | ||
|
|
||
| // This specifically tests out cases where we _cannot_ reuse DOM on shuffle. |
There was a problem hiding this comment.
This was tricky! A bit harder to hit this case now that we do more DOM re-use here. Again, just making sure we maintain 100% coverage.
theengineear
force-pushed
the
use-move-before
branch
from
a574389 to
71a4955
Compare
| }); | ||
|
|
||
| // TODO: #254: See https://chromestatus.com/feature/5135990159835136. | ||
| it.todo('native map does not cause disconnectedCallback on list shuffle', () => { |
There was a problem hiding this comment.
Very satisfying to delete this “.todo” here and see it just work ❤️
| /** | ||
| * Extends HTMLElement.prototype.connectedMoveCallback. | ||
| */ | ||
| connectedMoveCallback() {} |
There was a problem hiding this comment.
Minor interface addition here for discoverability (matches our other patterns — i.e., you should be able to rely on super existing).
theengineear
force-pushed
the
use-move-before
branch
from
71a4955 to
28e0b44
Compare
| // Note that passing “null” as the reference node moves nodes to the end. | ||
| for (let iii = nodes.length - 1; iii >= 0; iii--) { | ||
| const node = nodes[iii]; | ||
| parentNode.moveBefore(node, referenceNode); |
There was a problem hiding this comment.
This is really the key change. We leverage the new moveBefore API to move these items without disconnecting / reconnecting them!
|
@klebba — Here’s that |
Previously, both “connectedCallback” and “disconnectedCallback” would be called when simply attempting to reshuffle elements in a mapping within the same DOM tree. With the addition of the “moveBefore” element API, we can fix this! Now, such children are simply _moved_ and remain connected. Additionally, a new test was added to confirm that browsers are calling the “connectedMoveCallback” method on moves. And, another test added to ensure that we are testing all conditions / ternaries in our code to maintain 100% coverage. Closes #254.
theengineear
force-pushed
the
use-move-before
branch
from
28e0b44 to
c6c089f
Compare
|
Update — going to convert this to a draft PR until we see support in Safari. It’ll sorta just be ready to roll, but no need to act too hastily here. |
Previously, both “connectedCallback” and “disconnectedCallback” would be called when simply attempting to reshuffle elements in a mapping within the same DOM tree.
With the addition of the “moveBefore” element API, we can fix this! Now, such children are simply moved and remain connected.
Additionally, a new test was added to confirm that browsers are calling the “connectedMoveCallback” method on moves. And, another test added to ensure that we are testing all conditions / ternaries in our code to maintain 100% coverage.
Closes #254.