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 CSS-based theming on the canvas #13828

Merged
merged 40 commits into from May 3, 2024

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Apr 16, 2024

This PR:

  • associates DOM hierarchy with canvas renderers (one element per renderer, including glyph renderers)
  • allows visuals (fill, line, text, hatch) to consult CSS stylesheets for visual property values

An example showing CSS styling of plot background and axis background, ticks, labels and line (see examples/styling/visuals/css_variables.py):

image

Having an associated DOM hierarchy will aid debugging on the canvas, as well as set up ground work for accessibility/ARIA support and perhaps fully CSS-based layout in the future:

image

Performance implications of the DOM hierarchy should be negligible.

@mattpap mattpap added this to the 3.5 milestone Apr 16, 2024
Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.62%. Comparing base (e61219b) to head (a53da8c).
Report is 21 commits behind head on branch-3.5.

❗ Current head a53da8c differs from pull request most recent head 55d9deb. Consider uploading reports for the commit 55d9deb to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##           branch-3.5   #13828      +/-   ##
==============================================
+ Coverage       91.57%   92.62%   +1.04%     
==============================================
  Files             326      326              
  Lines           20737    20767      +30     
==============================================
+ Hits            18990    19235     +245     
+ Misses           1747     1532     -215     

@mattpap
Copy link
Contributor Author

mattpap commented Apr 22, 2024

This is tentatively ready for review. There is a class of failures (resulting in multiple integration test errors) that I'm fighting with, which a side effect of bad design around DOMView.render() and DOMView.after_render(), but fixing that is unrelated to any relevant changes in this PR.

bokehjs/src/lib/core/dom.ts Outdated Show resolved Hide resolved
src/bokeh/models/ui/ui_element.py Show resolved Hide resolved
bokehjs/src/lib/core/ui_gestures.ts Show resolved Hide resolved
bokehjs/src/lib/core/util/menus.ts Show resolved Hide resolved
bokehjs/src/lib/core/visuals/visual.ts Outdated Show resolved Hide resolved
@bryevdv
Copy link
Member

bryevdv commented Apr 23, 2024

Approved with some minor inline comments.

Some other general comments:

  • it would be really helpful if you can provide an example of embedding in a page that makes use of or refers to CSS in the template, not defined using Bokeh APIs
  • it would have been very helpful to reviewers if this PR had been split up. Any/all of these would have made sense as standalone PRs, would have been trivial to approve very quickly, and would have made this PR also much smaller and faster to review as well:
    • BokehJS util func deprecations/removals
    • paint/render rename
    • adding status-quo versions new visuals's APIs (get_line_color etc) to have a single place to insert CSS changes in this PR
    • render_group move and StyledElement refactor

Does the generated DOM hierarchy include the model names, so that those could be used as selectors in some way? A purely type based option is a bit limiting if users want to style different axes on different plots, say.

Lastly, I guess the specificity of e.g. bk-LinearAxis is unavoidable? It would be nice if somehow just bk-Axis could be used for any axis type, but I don't really see a path to that.

@mattpap
Copy link
Contributor Author

mattpap commented Apr 23, 2024

Lastly, I guess the specificity of e.g. bk-LinearAxis is unavoidable? It would be nice if somehow just bk-Axis could be used for any axis type, but I don't really see a path to that.

That specificity is there by design, because those classes are added for ease of identification of DOM components and thus ease of debugging. They are not meant for user consumption and they really can't be used for styling, because shadow DOM would prevent users from attaching any global or global-ish styling using those classes (this is why we are migrating to using CSS variables).

@mattpap mattpap force-pushed the mattpap/12759_css_theming branch from e228941 to 960557d Compare May 1, 2024 08:54
@mattpap mattpap force-pushed the mattpap/12759_css_theming branch from 960557d to 6e709ed Compare May 2, 2024 16:53
@mattpap mattpap force-pushed the mattpap/12759_css_theming branch from 6e709ed to cf8dc3e Compare May 3, 2024 06:08
@mattpap mattpap merged commit 9aaa141 into branch-3.5 May 3, 2024
25 checks passed
@mattpap mattpap deleted the mattpap/12759_css_theming branch May 3, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants