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

sizing mode scale_both grows on window resize #7978

Closed
Karel-van-de-Plassche opened this Issue Jun 8, 2018 · 37 comments

Comments

Projects
None yet
3 participants
@Karel-van-de-Plassche
Copy link
Contributor

Karel-van-de-Plassche commented Jun 8, 2018

I'm building a layout with nested rows and columns using the new HTML template feature and bootstrap/flexbox. I would expect that the bokeh plots would just fill the available space. However, the plots start small and grow on every window resize, even though the bootstrap columns/rows resize correctly.
ezgif com-gif-maker

from IPython import embed
from bokeh.layouts import row, column, layout, gridplot, Spacer, widgetbox
from bokeh.io import curdoc
from bokeh.models.widgets import Slider
from bokeh.plotting import figure
from bokeh.embed import components

size = 500⋅
figs = []
sizing_mode = 'scale_both'
for ii in range(4):
    fig = figure(plot_width=int(size), plot_height=int(size), name='freq' + str(ii), sizing_mode=sizing_mode)
    fig.circle(0, 0, radius=1, fill_color="green")
    figs.append(fig)
for ii in range(3):
    fig = figure(plot_width=int(size), plot_height=int(size), name='flux' + str(ii), sizing_mode=sizing_mode, tags=['fluxlike'])
    fig.circle(0, 0, radius=1, fill_color="red")
    figs.append(fig)

sliders = []
for ii in range(9):
    sliders.append(Slider(start=0, end=5))

slidercol1 = column(widgetbox(*sliders[:5], height=20, width=size, sizing_mode='scale_width'), name='sliders1')
slidercol2 = column(widgetbox(*sliders[5:], height=20, width=size, sizing_mode='scale_width'), name='sliders2')
sliderrow = row(slidercol1, slidercol2, sizing_mode='scale_width')

for fig in figs:
    curdoc().add_root(fig)

curdoc().add_root(slidercol1)
curdoc().add_root(slidercol2)
{% extends base %}
{% block preamble %}
<script src="https://code.jquery.com/jquery-3.3.1.slim.min.js" integrity="sha384-q8i/X+965DzO0rT7abK41JStQIAqVgRVzpbzo5smXKp4YfRvH+8abtTE1Pi6jizo" crossorigin="anonymous"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.3/umd/popper.min.js" integrity="sha384-ZMP7rVo3mIykV+2+9J3UJ46jBk0WLaUAdn689aCwoqbBJiSnjAK/l8WvCWPIPm49" crossorigin="anonymous"></script>
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.1/css/bootstrap.min.css" integrity="sha384-WskhaSGFgHYWDcbwN70/dfYBj47jz9qbsMId/iRN3ewGhXQFZCSftd1LZCfmhktB" crossorigin="anonymous">
<script src="https://stackpath.bootstrapcdn.com/bootstrap/4.1.1/js/bootstrap.min.js" integrity="sha384-smHYKdLADwkXOn1EmN1qk/HfnUcbVRZyYmZ4qpPea6sjB/pTJ0euyQp0Mk8ck+5T" crossorigin="anonymous"></script>
<style>
.p0 { background-color: red; }
.p1 { background-color: green; }
.s0 { background-color: green; }
.s1 { background-color: blue; }

.container-fluid {
  flex-wrap: nowrap;
  height: 50vh;
}

.col {
  flex-wrap: nowrap;
  flex-basis: auto;
  flex-shrink: 1;
  border: 2px solid red;
  margin: 1px;
  min-width: 0;
}

.square:before {
  content:'';
  float:left;
  padding-top:100%;
}

.row {
  flex-wrap: nowrap;
}

.bigrow {
  display:flex;
}
</style>

{% endblock %}
{% block contents %}
  <div class="container-fluid">
    <div class="bigrow">

      {% for root in roots %}
        {% if "fluxlike" in root.tags %}
          <div class="col square">
            {{ embed(root) }}
          </div>
        {% endif %}
      {% endfor %}

      <div class="col square">
        <div class="row">
          <div class="col square">
            {{ embed(roots.freq0) }}
          </div>
          <div class="col square">
            {{ embed(roots.freq1) }}
          </div>
        </div> {# row #}
        <div class="row">
          <div class="col square">
            {{ embed(roots.freq2) }}
          </div>
          <div class="col square">
            {{ embed(roots.freq3) }}
          </div>
        </div> {# row #}
      </div> {# col square #}
    </div> {# bigrow #}


    <div class="bigrow">
      <div class="col">
        {{ embed(roots.sliders1) }}
      </div>
      <div class="col">
        {{ embed(roots.sliders2) }}
      </div>
    </div> {# bigrow #}
  </div> {# container-fluid #}
{% endblock %}

Python version : 3.6.5 | packaged by conda-forge | (default, Apr 6 2018, 13:39:56)
IPython version : 6.3.1
Bokeh version : 0.13.0dev8-dirty
BokehJS static path : /home/karel/working/bokeh/bokeh/server/static
node.js version : v8.10.0
npm version : 6.1.0

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 8, 2018

PS. seems related to this comment: #7270 (comment)
./examples/embed/embed_responsive_width_height.py does the same!

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 8, 2018

scale_width seems to work well though!

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 8, 2018

Yah I had noticed this and thought there was an issue but evidently just a comment. Thanks for the detailed report!

@Karel-van-de-Plassche Karel-van-de-Plassche changed the title sizing mode scale_both in bootstrap column grows on window resize sizing mode scale_both grows on window resize Jun 8, 2018

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 8, 2018

@mattpap can you look at this one? This is an important mode for use with the new template options. My hope is that it is some dumb simple fix.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

I'm not sure what's up, when I run my example script from the PR I get a page that works but when I run your code above on master @Karel-van-de-Plassche no plots show up for me. (I see the red outline of the bs divs, but no plots) Are plots showing up for you on master?

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

@mattpap This diff seems to fix things:

diff --git a/bokehjs/src/lib/models/layouts/layout_dom.ts b/bokehjs/src/lib/models/layouts/layout_dom.ts
index cf145939f..9c86ec860 100644
--- a/bokehjs/src/lib/models/layouts/layout_dom.ts
+++ b/bokehjs/src/lib/models/layouts/layout_dom.ts
@@ -338,7 +338,7 @@ export abstract class LayoutDOMView extends DOMView {

     let width: number
     let height: number
-    if (new_width_1 < new_width_2) {
+    if (new_width_1 > new_width_2) {
       width = new_width_1
       height = new_height_1
     } else {

The reason has something to do with the fact that parentHeight is always reporting zero, so with the original condition the second branch in get_width_height was always taken which caused it to always report 0 for both. Not sure if this is the actual correct solution though. Please comment as soon as you can.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

Definitely not everything is perfect with that change:

screen shot 2018-06-10 at 20 57 30

presumably if theclientHeight was accurately reported, then those plots would fit in the actual heigh of the overall layout properly.

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

@bryevdv
I'm a bit confused. For both master and 0.13.0dev8 the plots don't show up. I'm not sure what happened in my 0.13.0dev8-dirty from the original report that made the plots show up..

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

@bryevdv Silly me, I just copied the wrong template over to the issue! I've edited to original post with the correct one which shows the plots.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

It failed because earlier you didn't have 1-1 mapping between roots and embedding points in the template.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

if (new_width_1 > new_width_2) {

Well, you could remove the other branch altogether, knowing that new_{width,height}_2 are always zero. Though, if I'm not mistaken, now scale_both behaves essentially like scale_width. Without accurate reporting of available space in both dimensions, there isn't really much to be fixed here.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

I somewhat agree, tho had hope you might have some insight as to why clientHeight is not coming back properly.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

@birdsarah do you have any insights about clientHeight always showing zero?

FWIW offsetHeight also always seems to be zero

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

It failed because earlier you didn't have 1-1 mapping between roots and embedding points in the template.

Yeah, I noticed, thanks!

The reason has something to do with the fact that parentHeight is always reporting zero

What is the parent in this case? The bk-root? Related question, as someone trying to learn bokeh a bit more deeper, what is the purpose of this element? Can't it just enclose all it's children? Or does that mess up some further formatting?

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

(...) and hope you might have some insight as to why clientHeight is not coming back properly.

There may be two reasons for this. Either lack of "right" CSS, that would allow getting such measurements (positioning and what not), or employing DOM where we should be asking layout (asking DOM makes sense only for roots).

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

What is the parent in this case? The bk-root?

It depends where a model is in the layout. If it is the root (of a layout) then its parent is bk-root (as of master, because earlier this was more complicated).

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

It depends where a model is in the layout. If it is the root (of a layout) then its parent is bk-root (as of master, because earlier this was more complicated).

Okay. Currently the bk-root always has height of 0, right? Could this special case then be solved by letting the root scale with the size of the underlying children? If not, what is the underlying design philosophy of the bk-root?

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

what is the purpose of this element

The main purpose of bk-root is to create a namespace for bokeh's CSS (secondary to bk- prefixing). This is necessary to deal with limitations of CSS. Originally we intended to have one bk-root per HTML document, but that wasn't flexible for the user and caused issues with computing layout for responsive modes. So we are gravitating towards having one bk-root per root model. After recent "templating" PR, there is essentially 1-1 correspondence between bk-root elements and roots. I'm saying essentially, because there are corner cases (e.g. date picker widget). One can view bk-root, besides the namespacing role, as an interface between DOM and managed DOM (as in "managed by layout").

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

So, if it's going to be broken for the immediate term, we could merge a change to just use the first branch always. That kind of wrong seems at least useful in some cases (where the width is larger) as opposed to the "ever growing" behaviour now, which is never useful.

But then longer term we need to figure out how to fix scale_both or get rid of it if we can't, I guess.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

As a temporary measure with can proceed with this. After all, scale_both is barely referenced anywhere in bokeh. Given its fragile nature, this code was and wasn't working on and off for a long time.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

If I have to guess when it was broken again, I'm pretty sure it was when I removed the requirement for setting width and height of certain DOM elements in every template.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

If I have to guess when it was broken again, I'm pretty sure it was when I removed the requirement for setting width and height of certain DOM elements in every template.

That makes sense Can you estimate how much effort to investigate/fix that? I will go ahead with the PR described above for now, in any case.

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

Thanks for the explanation. With the new HTML templates, I think a working 'fill parent div' option would be nice.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

I think a working 'fill parent div' option would be nice.

@Karel-van-de-Plassche , what do you mean by that?

@mattpap mattpap modified the milestones: 0.13.0, 0.13.x Jun 11, 2018

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

what do you mean by that?

Working scale_both and/or stretch_both I assume

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

@bryevdv Yeah, that was what I meant, sorry if I was unclear. Both scale_both and stretch_both would be nice, especially with the fine-grained control the new HTML templates give you. For a plot to be able to fill (and not overflow) the HTML div you put it in would be great!

Cannot check right now, but I suppose both suffer from being unable to (correctly) query the parent for sizes? Maybe we can have a quick workaround in the templates/index.html itself?

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

That makes sense. Given the context of HTML templates and roots, I though you might have had an idea for a different feature.

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

For now I'm extremely happy with the features already there ;)

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

Maybe we can have a quick workaround in the (...)

In fact there is a workaround:

template="""\
{% block postamble %}
<style>
html, body, .bk-root {
    width: 100%;
    height: 100%;
    margin: 0;
}
</style>
{% endblock %}
"""

I actually mentioned it in #7978 (comment), but didn't connect the dots. I wouldn't even consider this a workaround, but what should be a part of a template to make scale_both (and possibly stretch_both) to work. In general LayoutDOM.get_width_height() should get the same changes as LayoutDOM._calc_width_height() got (perhaps those methods should be merged). However, this applies to roots. scale_both and stretch_both are not fully well defined for child models, so your mileage may vary with complex layouts.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

@mattpap can we put that in our base template? Then we could revert my earlier PR, as long as we also document the need to do this in "custom" templates with scale/stretch, if the base template is not used.

Thoughts? We need to decide and act soon, I'd still like to release in the next few days.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

Given all this, the "incorrect" implementation is actually correct but it assumes certain properties about the enclosing DOM and CSS. I though I already lifted those assumptions in an earlier PR, but I overlooked get_width_height().

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

can we put that in our base template?

That would invalidate earlier work and I wouldn't be too happy with bringing this back. However, I will see if I can make get_width_height() work on the same premise as _calc_width_height() does. That would solve all the issues.

@Karel-van-de-Plassche

This comment has been minimized.

Copy link
Contributor Author

Karel-van-de-Plassche commented Jun 11, 2018

I think it is fair to assume that anyone who wants the fine-grained control of an HTML template has to put every plot in its own root with the given 'workaround' such that scale_both and stretch_both work. This could maybe get documented on a 'HTML templates' page.

I'm not sure how widely HTML templates will be used, so I would avoid breaking something else for HTML templates to work.

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

That's true, but to make those modes work in general, we would have change the main template, as @bryevdv mentioned. Previously it was done, because it was a cheap and easy solution, but doing so is wrong for a library and fragile in general.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

@mattpap OK let's hold off on the release a few days and see if you can get this working by wednesday morning or so (PST). I will start writing up the blog post

@mattpap

This comment has been minimized.

Copy link
Contributor

mattpap commented Jun 11, 2018

@bryevdv, it's fixed

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Jun 11, 2018

@mattpap fantastic I will merge the PR soon and get another dev build out for testing and test some tonight (and make some new examples for docs)

@bryevdv bryevdv modified the milestones: 0.13.x, 0.13.0 Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment