Skip to content

Fix positioning of DOM rendered Legend annotations#14457

Merged
mattpap merged 13 commits into
branch-3.8from
mattpap/14451_Legend_position
May 7, 2025
Merged

Fix positioning of DOM rendered Legend annotations#14457
mattpap merged 13 commits into
branch-3.8from
mattpap/14451_Legend_position

Conversation

@mattpap
Copy link
Copy Markdown
Contributor

@mattpap mattpap commented Apr 15, 2025

This PR makes ToolbarPanel and Title participate in CSS layout. This way all side panel components can be positioned consistently. I attempted to make a minimal change that fixes the issue and can be back-ported. From here we can pretty much get rid of computed layout in side panels.

fixes #14451

@mattpap mattpap added this to the 3.8 milestone Apr 15, 2025
@mattpap mattpap mentioned this pull request Apr 24, 2025
13 tasks
@mattpap mattpap force-pushed the mattpap/14451_Legend_position branch 4 times, most recently from 0493279 to e8ce81d Compare April 28, 2025 00:15
@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented Apr 28, 2025

This is a bit much for a backportable PR, but given how badly positioning was broken in 3.7, then we don't have much choice in the matter.

@mattpap mattpap requested a review from hoxbro April 29, 2025 14:15
@hoxbro
Copy link
Copy Markdown
Contributor

hoxbro commented Apr 29, 2025

I have two examples that indicate things are still broken.


Example 1 - missing toolbar after adding a line [works on Branch-3.8 (a6f66f5), broken with this PR (e8ce81d)]

Code
from bokeh.plotting import figure, curdoc
from bokeh.models import Button
from bokeh.layouts import column

x = [1, 2, 3, 4, 5]
y = [6, 7, 2, 4, 5]

p = figure()

def add_line(count=[0]):
    p.line(x, [val + count[0] for val in y])
    count[0] += 1

add_button = Button(label="Add Line", button_type="success")
add_button.on_click(add_line)

# Layout and add to document
curdoc().add_root(column(p, add_button))

Running from this PR:

Screencast.From.2025-04-29.18-55-09.mp4

Example 2 - plot and legend split from each other [works on Branch-3.6 (62ace62), broken with this PR (e8ce81d), Branch-3.7 (119bb23), Branch-3.8 (a6f66f5)]

Code
from bokeh.plotting import figure, curdoc
from bokeh.models import Button, Legend
from bokeh.layouts import column

x = [1, 2, 3, 4, 5]
y1 = [6, 7, 2, 4, 5]
y2 = [1, 4, 3, 2, 6]

p = figure()
line1 = p.line(x, y1, color="red")
line2 = p.line(x, y2, color="blue")
legend = Legend(
    items=[("Line 1", [line1]), ("Line 2", [line2])],
)
p.add_layout(legend, "right")


def swap_lines():
    idx1 = p.renderers.index(line1)
    idx2 = p.renderers.index(line2)
    p.renderers[idx1], p.renderers[idx2] = p.renderers[idx2], p.renderers[idx1]

    legend.items = [
        ("Line 1", [line1]) if p.renderers[0] is line1 else ("Line 1", [line2]),
        ("Line 2", [line2]) if p.renderers[1] is line2 else ("Line 2", [line1]),
    ]


swap_button = Button(label="Swap Line Order", button_type="primary")
swap_button.on_click(swap_lines)

curdoc().add_root(column(p, swap_button))

Running from this PR:

Screencast.From.2025-04-29.18-57-10.mp4

Running from branch-3.8:

Screencast.From.2025-04-29.18-59-18.mp4

@mattpap mattpap force-pushed the mattpap/14451_Legend_position branch from e8ce81d to 83be873 Compare May 7, 2025 12:22
@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented May 7, 2025

@hoxbro, I fixed all problems found so far.

@maximlt maximlt mentioned this pull request May 7, 2025
12 tasks
@hoxbro
Copy link
Copy Markdown
Contributor

hoxbro commented May 7, 2025

I think what @maximlt is seeing is adding a legend two times. Previously, it would attach to the right; now, it attaches to the center.

Branch adding center first adding right first
branch-3.6 image image
branch-3.8 image image
mattpap/14451_Legend_position image image
from bokeh.models import Legend, LegendItem
from bokeh.plotting import figure, curdoc

p = figure(frame_width=300, frame_height=300)
r1 = p.line([1, 2, 3], [4, 5, 6])

legend = Legend(items=[LegendItem(label="Line 1", renderers=[r1])])
legend.location = (0, 0)
p.add_layout(legend, "right")
p.add_layout(legend, "center")

curdoc().add_root(p)

@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented May 7, 2025

Previously, it would attach to the right; now, it attaches to the center.

To me, this is a better behavior, though perhaps coincidental. The right behavior is to have the legend rendered twice, but that's currently limited by a single view storage for all panels of a plot. This will be resolved in my ongoing work to support generic layouts in plot's panels.

Currently Plot.add_layout() simply pushes to the chosen panel, disregarding previously pushed instances. We can settle on whatever behavior we choose. We could remove existing instances, sealing the current behavior, or ignore new instances, restoring the old behavior, or throw an error if trying to repush an instance.

@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented May 7, 2025

Oh, it always ends up in the center panel. Well, that's definitely a bug.

image

@mattpap
Copy link
Copy Markdown
Contributor Author

mattpap commented May 7, 2025

@hoxbro, @maximlt, I made Plot.add_layout() move an object into the new location. This still can be messed up when using left, right, etc. properties manually. This will be fixed later.

Copy link
Copy Markdown
Contributor

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some comments, but it seems to have solved all the cases I could think of.

(Last set of changes also fixed @maximlt MRE)

Comment thread bokehjs/src/less/canvas.less
Comment thread bokehjs/src/lib/models/annotations/toolbar_panel.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the extra space below deliberate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean at the bottom of the image? It's just generous "viewport" area. Could be tightened though. Images are captured by this viewport, which is either manually or automatically computed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is OK with me as long as it is not the element that causes it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if test's output exceeds the prescribed viewport size, then the test will fail. The default is to give 50 px margin on both sides. In this particular case, the viewport has to be manually computed, because the test uses frame_width and frame_height properties, which aren't supported in the automated method.

Comment thread src/bokeh/models/plots.py Outdated
@mattpap mattpap force-pushed the mattpap/14451_Legend_position branch from 3cc9d24 to 4b4132b Compare May 7, 2025 17:35
@mattpap mattpap merged commit 8936020 into branch-3.8 May 7, 2025
24 checks passed
@mattpap mattpap deleted the mattpap/14451_Legend_position branch May 7, 2025 18:06
mattpap added a commit that referenced this pull request May 7, 2025
* 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
@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
* 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>
@github-actions
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legend overlaps toolbar when located outside plot

2 participants