Size of histogram is miscalculated, too big overflows it's parent

[jrochkind]

The JS drawing the histogram need sto be told it's width/height. There is some JS code in blacklight_range_limit that tries to calculate what it should be based on how much space is available.

This is currently failing in some circumstances. And you get something like this:

For me, this happens whenever I am loading a page with an existing range limit applied, so it opens with that facet section starting out open. Clicking on the facet section header to close and then re-open the section -- actually triggers a recalculation/redisplay and corrects it's size.

For me, on MacOS Chrome, this is pretty reproducible. Pretty much every time I load a page with an existing facet limit, it happens. However, on the very same app, other people on other browser/platform combos sometimes don't see the problem.

@pmulak on Slack also independently observed the problem, and said it reproduced on Firefox Chrome. Here where I am, we have one Windows Chrome that does reproduce it, and one that doens't. I think it's not actually browser-specific, but just hard to reproduce for as of yet mysterious reasons.

Somehow at the point the JS code is trying to determine the on-screen layout space available for the chart, it is not succesfully getting the right value. I previously spent some time investigating and was not able to figure it out, it's some creaky code (that i probably wrote originally many years ago). But I am now going to spend more time, and will try to leave notes here.

[jrochkind]

OK, another weird thing I can't yet explain:

If I do a hard-refresh in the browser on a page that reproduces -- it stops reproducing the problem.

If I load the page normally, it does reproduce the problem. Clearly has something to do with timing of the JS loads.

[jrochkind]

Some more things I am remembering/discovering investigating the code.

We use flot to the draw the plot. It will draw a plot up to within the bounds of it's container. But if the container does not have an explicit height, it seems to complain eg Invalid dimensions for plot, width = 201, height = 0.

So we have some interesting code around here: https://github.com/projectblacklight/blacklight_range_limit/blob/master/app/assets/javascripts/blacklight_range_limit/range_limit_distro_facets.js#L129-L142

Which tries to:

1. Wait until the container div (which is the one you can find with selector .distribution.subsection.chart_js) has a non-zero width -- it's waiting for layout to be gone far enough that it has a width, and if it's not yet, it'll set a timeout and try again 50ms later, up to timeout.
2. THEN calculate a desired height based on an aspect ratio, and set the container to have that height as an inline style
3. Then tell flot to draw the chart.

(There might be a better way to do all this, but this is what the code has been doing for years).

In my app, that container div eventually settles down to 201px wide. However, there is some point where the javascript using JQuery width() to get it' width -- gets a number that's more than 1000px (maybe it's just my viewport width, the thing has become total viewport somehow?). It sets the height accordingly, and has flot draw the chart -- at this much too big size.

Later the container is only 201px, but the chart is already drawn.

Not sure why the container is temporarily 1000px+ instead. This is definitely kind of hacky code; I wonder if there's an entirely better way to do all this code using modern browser possibilities, but it might be a pretty major rewrite. This gem could probably use a pretty major rewrite, but meanwhile I continue to investigate to see what i can do.

[jrochkind]

OK, the code for sizing the chart fires in Blacklight.onload which in many cases is basically the same as DOMContentLoaded/JQuery document/ready. These are all slightly differnet but are all about the same, approx DOMContentLoaded. (ignoring turbolinks; so far I am reproducing and troubleshooting only without turbolinks).

But they ALL can happen before CSS stylesheets are even loaded. There is no guarantee that the page layout is actually complete when the event fires. (refs https://developer.mozilla.org/en-US/docs/Web/API/Window/DOMContentLoaded_event https://javascript.info/onload-ondomcontentloaded#readystate)

So somehow, the event is firing when in some cases the div's don't really have their final size yet; they have a non-0 size for some reason, but have a size that equals the entire widht of the viewport. Later they will have the "right" size, but we don't wait long enough, and it's not entirely clear if there's any good way to wait long enough.

Still don't understand exactly how this is possible, since my stylesheets are loaded in html head... but I guess the browser loads them (or at least applies them) async for some reason?

This whole design is kind of problematic and fragile; but it's unclear how to fix it. Still looking for at least hacky fixes.

[jrochkind]

My own app in which I'm reproducing the problem loads it's JS with defer: true. @pmulak, I am wondering if same is true for you?

I am not entirely sure if removing that makes problem go away; of course, we'd like to be able to do that.

It looks like an ordinary script tag will not execute until previously loaded stylesheets are parsed; but a script tag with defer can. That could result in the error condition being impossible with synchronous script, but now a race condition where it is possible with defer (same would be true of async). See https://stackoverflow.com/a/42951150/307106

[jrochkind]

OK, yes.

1. We have to tell flot the height/width dimensions to render the plot

2. We have a variety of hooks in JS code to try to resize it on browser window resize, on facet collapse open/close, etc.

3. But for the initial display of a date range in a facet collapsible that is already open (because it already had a selection so defaults to open), it just does it as soon as it finds it -- in a Blacklight.onload. Which is basically DOMContentLoaded plus turbolinks events. There is logic to wait until the area has a non-0 width, because of previous cases where it was 'too early' and it had a 0 width.

4. But there are some cases DOMContentLoaded fires when the sidebar area doesn't really have it's "resting" size yet, but doesn't have a 0 width, it seems to have a has a full viewport width size.

   • This may be ONLY if you have your <script> tag set to defer and/or async, or it may be a lot more likely if you have defer/async. I was unable to reproduce without defer/async. I'm not sure if BL by default generates with defer and/or async, I think not? But it is a reasonable thing to want to do that shouldn't break things.

• How likely this is to happen or whether it can may depend on how many layout reflows happen in BL loading and the particular DOM, so it's not too shocking if a new BL version with refactored DOM/JS/CSS would change it.

It's not entirely clear to me how to fix this or the best way to minimize it.

• If there were a way to subscribe to an event for "layout/reflow done", that'd be the right time to draw/size the chart. But it's really never "done", and there is no web api to subscribe to "layout/reflow" events (perhaps cause it would risk infinite loop as your handler could cause one!)

• If there was a way to not have to tell flot the exact size, but just get it to generate a canvas that was 100% of parent an automatically reflowed, that'd be great. Or something other than flot? Not sure if there is, something other than flot would of course require some major rewriting, I am not pursuing either of these.

• if the range facet didn't start out open even if it there was a selected range limit, everything would work right -- by the time you can see it, it probably has the right size, and there's a draw/resize triggered on facet expand already. Unclear how we could make this so, or if it'd be desirable?

• In my experiments while the DOMContentLoaded event is sometimes too early, body.onload never is.

  • However, Blacklight.onload gives us a nice turbolinks-friendly way to hook into DOMContentLoaded, we don't have a pre-built way without reinventing the wheel to make it load+turbolinks instead.
  • And load is probably sometimes way too late. It doesn't fire until all images have loaded.

• We can add an additional hook, it keeps doing everything it's doing, but ALSO does a chart resize on body.onload. I did this by adding around here:

        window.addEventListener('load', function(event) {
          $(".chart_js").each(function(i, container) {
            redrawPlot($(container));
          });
        });
  

  That SEEMED to fix up my use case, I was unable to reproduce the problem once adding it. I don't think it'll create any problems -- although it is an additional 'resize' operation, telling flot to redraw the chart when it may not be needed. (Could actually be extended with a conditional to only do it if the size seems to have changed?)

  I can't promise it will fix or work right in all use cases. This is all pretty hacky, and really needs a whole different approach probably, but that's outside my available time/energy right now.

Any thoughts or feedback from anyone else welcome!

[jrochkind]

And here's a really hacky workaround that works in my own local app, with no changes to blacklight_range_limit.

We want to trigger a chart redraw on body.onload. But all that code is locked within blacklight_range_limit closure. But blacklight_range_limit will already respond to a shown.bs.collapse event with a chart redraw/resize.

So we can send a fake shown.bs.collapse on the collapsible element for the facet(s) that contain the blacklight_range_limit chart. In my case, the div that has a bootstrap collapsible for that facet winds up with id facet-year_facet_isim

So in my local code:

window.addEventListener('load', function(event) {
  $("#facet-year_facet_isim").trigger("shown.bs.collapse");
});

Seems to solve the problem for me, doesn't seem to cause any problems even with the facet has no pre-existing limit and begins collapsed. VERY HACKY.

[pghorpade]

Thanks @jrochkind , I am going to try some of the workaround solutions you provided.

[dkinzer]

Just confirming that @jrochkind hack works for us though we used $("#facet-pub_date_sort").trigger("shown.bs.collapse");