Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for canvas and DOM rendering to Legend #14028

Merged
merged 43 commits into from
Oct 30, 2024

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Aug 16, 2024

The goal of this PR is allow both canvas and DOM/CSS rendering for renderers for which it makes sense, i.e. renderers that are UI components and renderers that include text. Secondary goal is to replace custom layouts with CSS layout where every possible without a major redesign. I expect only a selected subset of renderers will be covered in this PR. I started with Legend because it touches every aspect that I need to cover by the new design.

The effects of this PR will be:

  • better integration with DOM/CSS (e.g. responsiveness to font loading/changes)
  • better theming and customizability beyond what visuals offer (no support for save/copy tool in this case)
  • support for text selection without the need for any custom implementations and new tools
  • ease of development (renderers are inspectable in devtools)
  • anything related to accessibility
Screencast.from.16.08.2024.11.49.27.webm

@mattpap mattpap added status: WIP grant: CZI R6 Funded by CZI Round 6 grant labels Aug 16, 2024
@mattpap mattpap added this to the 3.6 milestone Aug 16, 2024
@bryevdv
Copy link
Member

bryevdv commented Aug 16, 2024

@mattpap should the linked issues be triaged as feature? As an aside, I'd make a pitch for colorbar to be the next target for this kind of treatment.

@mattpap
Copy link
Contributor Author

mattpap commented Aug 21, 2024

should the linked issues be triaged as feature?

Yes. I'm actually triaging issues that may be affected by this work.

As an aside, I'd make a pitch for colorbar to be the next target for this kind of treatment.

Axes and title will have to be next. Then it will be possible to work on color bars.

@mattpap mattpap force-pushed the mattpap/10299_DOM_renderers branch 3 times, most recently from 102d16f to 97d3708 Compare September 16, 2024 00:12
@mattpap mattpap modified the milestones: 3.6, 3.7 Sep 16, 2024
@mattpap
Copy link
Contributor Author

mattpap commented Sep 17, 2024

Legend with DOM (left) and canvas rendering (right) side-by-side:
Screenshot from 2024-09-17 13-45-09

@mattpap
Copy link
Contributor Author

mattpap commented Sep 17, 2024

I was hoping to finish this PR as a big feature for bokeh 3.6, but that's unrealistic given how much additional work needed to be done to support DOM rendering and how much more is needed to tweak and fine tune the implementation. What needs to be done is:

  • fine tune CSS to cover all the corner cases (e.g. angled text)
  • address CSS cross-browser compatibility issues (e.g. vertical text in FF)
  • finalize support for positioning in side panels
  • don't render and paint at the same time (new output backend?)
  • address performance issues due to over-rendering (due to uncertainty when to update the legend)

@mattpap mattpap force-pushed the mattpap/10299_DOM_renderers branch 3 times, most recently from 7700aa7 to 5cf9c13 Compare September 24, 2024 12:35
@mattpap mattpap changed the base branch from branch-3.6 to branch-3.7 September 26, 2024 16:35
@mattpap mattpap force-pushed the mattpap/10299_DOM_renderers branch 4 times, most recently from c993e03 to 150f145 Compare October 23, 2024 07:20
@mattpap mattpap changed the title Add support for dual canvas and DOM/CSS rendering Add support for canvas and DOM rendering to Legend Oct 23, 2024
@mattpap
Copy link
Contributor Author

mattpap commented Oct 23, 2024

There is still one regression to address, but otherwise this is ready for review. Note that great many changes to the internals are more or less temporary, and will get refined in future PRs touching the subject. These will include changes to axes, labels, title and text glyphs.

@bryevdv
Copy link
Member

bryevdv commented Oct 29, 2024

@mattpap one of the images at least seems to have an issue?

Screenshot 2024-10-29 at 12 00 46

@mattpap
Copy link
Contributor Author

mattpap commented Oct 29, 2024

one of the images at least seems to have an issue?

It's fixed now.

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

I didn't have a chance to go over the the JS super carefully, but I did not see any obvious issues. I did go over the images quite carefully. @mattpap if you'd like more code review I'd encourage you to request from @philippjfr or @ianthomas23 directly. Otherwise, probably just better to get this merged earlier to see how it shakes out.

@mattpap
Copy link
Contributor Author

mattpap commented Oct 30, 2024

Test failure is unrelated and reported in issue #14128. I'm going ahead and merge this. There is still a lot of temporary stuff in this PR or stuff that require generalization, that when refined in hopefully in smaller PRs, will be easier for reviewers to digest.

@mattpap mattpap merged commit 5034bbc into branch-3.7 Oct 30, 2024
23 of 25 checks passed
@mattpap mattpap deleted the mattpap/10299_DOM_renderers branch October 30, 2024 18:55
@gabalafou
Copy link

I see that this has been merged and I have only given it a cursory look so far.

I was asked to check this work in Safari specifically. So I built the js and the docs with this PR applied and the first plot that I opened (transform markers) I noticed a pretty big difference between Chrome versus Safari.

Chrome:

image

Safari:

image

@mattpap
Copy link
Contributor Author

mattpap commented Oct 30, 2024

@gabalafou, I can reproduce this in epiphany (webkit on Linux). Can you start an issue for this? Although I can reproduce the issue, that browser is insufferable and I can't inspect or debug anything in it at this point at least.

@gabalafou
Copy link

I've started an issue here: #14134

I added the plat: safari and type: bug labels but I'm not entirely sure how to title and label this issue.

@mattpap
Copy link
Contributor Author

mattpap commented Oct 31, 2024

Thanks.

I added the plat: safari and type: bug labels but I'm not entirely sure how to title and label this issue.

That's a good start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grant: CZI R6 Funded by CZI Round 6 grant status: accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Legend outside plot area won’t resize Legend click policy on touchscreens
3 participants