Skip to content

Update child views without removing their elements#14459

Merged
mattpap merged 19 commits into
branch-3.8from
14458_update_children
Apr 24, 2025
Merged

Update child views without removing their elements#14459
mattpap merged 19 commits into
branch-3.8from
14458_update_children

Conversation

@philippjfr

@philippjfr philippjfr commented Apr 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR reimplements LayoutDOM.update_children and PaneView._update_elements to avoid the removal of DOM nodes before they are re-added.

@philippjfr philippjfr added this to the 3.8 milestone Apr 16, 2025
@philippjfr

philippjfr commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

I'd also love to see this in 3.7.3 but will leave that up to @mattpap

@philippjfr

philippjfr commented Apr 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @mattpap, would appreciate review. Alternatives here might be:

  1. Allow providing an index at which to insert at to render_to so we don't bypass that.
  2. Gathering all the children and calling replaceChildren at once (not a massive fan of this option since we'd have to gather all the style elements and whatever else as well).

philippjfr
philippjfr commented Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/layouts/layout_dom.ts Outdated
philippjfr
philippjfr commented Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/ui/pane.ts Outdated
philippjfr
philippjfr commented Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/layouts/layout_dom.ts Outdated
@philippjfr

philippjfr commented Apr 17, 2025

Copy link
Copy Markdown
Contributor Author

So I have determined that both of us were completely overthinking this. The removal of nodes was completely unnecessary because simply appending the children in order will move them to the correct place in the DOM (and actually be a complete no-op if they haven't moved at all). Unless you see some flaw in that thinking I think this is the nicest and simplest fix.

mattpap
mattpap reviewed Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/layouts/layout_dom.ts Outdated
philippjfr
philippjfr commented Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/layouts/layout_dom.ts Outdated
philippjfr
philippjfr commented Apr 17, 2025
View reviewed changes
Comment thread bokehjs/src/lib/models/ui/pane.ts Outdated
@mattpap

mattpap commented Apr 17, 2025

Copy link
Copy Markdown
Contributor

I added some rudimentary tests.

@mattpap mattpap changed the title Implement LayoutDOM.update_children without explicitly removing nodes Update child views without removing their elements Apr 24, 2025
@mattpap mattpap merged commit 3fd60e1 into branch-3.8 Apr 24, 2025
@mattpap mattpap deleted the 14458_update_children branch April 24, 2025 11:16
@mattpap mattpap mentioned this pull request Apr 24, 2025
Merged
13 tasks
mattpap added a commit that referenced this pull request Apr 24, 2025
@philippjfr @mattpap
Update child views without removing their elements (#14459)
6ed632e 
* Implement LayoutDOM.update_children without explicitly removing nodes

* Fix lint

* Fix logic

* More lint

* Simplify for loop

* No tabs

* Also update handling of element views

* Remove empty line

* Fix self_target children lookup

* Fix type errors

* Fix and simplify logic

* Fix element views

* Apply suggestions from code review

* Simplify further

* Fix lint

* Revert to append (instead of appendChild)

Rever

* Compare DOM nodes non-structurally by identity

* Add regression tests

* Unify all rebuilding of child views

---------

Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>
@mattpap mattpap linked an issue May 6, 2025 that may be closed by this pull request
Removing and re-adding DOM nodes causes React problems #14458
Closed
@mattpap mattpap modified the milestones: 3.8, 3.7.3 May 7, 2025
mattpap added a commit that referenced this pull request May 8, 2025
@mattpap @philippjfr
@mosc9575 @chinmaychahar
Backports for 3.7.3 (#14455)
f49bcc1 
* Update switcher.json

* Fix Legend's glyph rendering for `dpr != 1` (#14443)

* Fix Legend's glyph rendering for dpr != 1

* Allow to override screen scaling in testing

* Add regression tests

* Fix Legend's inactive visuals in CSS mode (#14454)

* Fix Legend's inactive visuals in CSS mode

* Add visual regression tests

* Update child views without removing their elements (#14459)

* Implement LayoutDOM.update_children without explicitly removing nodes

* Fix lint

* Fix logic

* More lint

* Simplify for loop

* No tabs

* Also update handling of element views

* Remove empty line

* Fix self_target children lookup

* Fix type errors

* Fix and simplify logic

* Fix element views

* Apply suggestions from code review

* Simplify further

* Fix lint

* Revert to append (instead of appendChild)

Rever

* Compare DOM nodes non-structurally by identity

* Add regression tests

* Unify all rebuilding of child views

---------

Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>

* update docs for DatetimeTickFormatter (#14452)

* Updated formatters.py

---------

Co-authored-by: Chinmay <chinmay.cc.06@gmail.com>

* fix links to code pen (#14471)

* add jquery to fix links to code pen

* remove jquery dependency

* remove patched show function

* use Node.COMMENT_NODE instead of number

* avoid line breaks in templates and code pen

* fix missing title

* Fix positioning of DOM rendered Legend annotations (#14457)

* Fix positioning of DOM rendered Legend annotations

* Treat inner canvas panels equally to outer

* Robustify resize of canvas after layout

* Update visual baselines

* Make sticky toolbar work correctly

* Refactor PlotView._update_layout()

* Invalidate layout if renderers change

* Add more regression tests

* Update visual baselines

* Always repaint Legend's glyphs after rendering

* Update visual baselines

* Implement move semantics in Plot.add_layout()

* Tighten regressions' baseline viewports

* Fix types of splattable figure's attributes (#14401)

* Fix types of splattable figure's attributes

* Add rudimentary typing tests

* Python 3.10 compatibility

* Improve corner case handling in datetime formatter (#14473)

* Add release notes

---------

Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Co-authored-by: Moritz Schreiber <68053396+mosc9575@users.noreply.github.com>
Co-authored-by: Chinmay <chinmay.cc.06@gmail.com>
@philippjfr philippjfr mentioned this pull request May 9, 2025
Merged
@github-actions

github-actions Bot commented Aug 20, 2025

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mattpap mattpap mattpap approved these changes

Assignees

No one assigned

Labels

status: accepted

Projects

None yet

Milestone

3.7.3

Development

Successfully merging this pull request may close these issues.

Removing and re-adding DOM nodes causes React problems

2 participants

@philippjfr @mattpap