Network with 100% height set does not render correctly

ghost commented 8 years ago

I'm trying to use Network where the height/width needs to be dynamic for a responsive layout.

If I set the Network to a fixed height/width all works as expected, except resizing (obviously) won't change the canvas to fit.

But if I set height/width to 100%, the canvas correctly sizes to the width (including autoResize set to true) but I always end up with a 150px height canvas, whereas the height of the container div could comfortably be > 800px.

I've created a demo plunker to show the problem.

abcbaby commented 8 years ago

I've made a tweak to your code; Basically, you can't use %, you need to "px". Here is the new code example from your fork, look at your script.js:

http://plnkr.co/edit/B73VN9Kbew9KfCzYxUKs?p=preview

ghost commented 8 years ago

Problem is this isn't dynamic - my understanding is that to get the network autoResize to work, it needs to be in % not px - otherwise it won't think to resize (as it's still rendering at the fixed width). I assume on resize I could try to recalculate, but I need it to fill 100% of the container, rather than window size.

I'm using this in bootstrap where it's inside a 9 col div (where the full 12 col set are nested inside a 9 col div). I know from prior experience that trying to render svg/canvas to fill 100% of a container is not the easiest thing though.

mojoaxel commented 7 years ago

I'm closing this issue due to inactivity. If you think this issue is still relevant please feel free to reopen!

pjeutr commented 7 years ago

I still see this happening with 4.20.1 Also using it within a bootstrap grid and see the canvas size set to height="150" initially I can overwrite it in options with something like 500px, but 100% is not working

pjeutr commented 7 years ago

http://plnkr.co/edit/2FGTOd8v2GbmwGgeYAVu?p=preview shows the problem. I also use GraphJs which scales fine. Part of the problem for me of getting unreadable results is that visjs takes full width but only 150px height. If there would be a definable aspect ratio or something like height = .8 * width. It would look a lot better for me.

wimrijnders commented 7 years ago

@pjeutr Thanks for demo, that's useful. Makes it much easier to examine the issue.

wimrijnders commented 7 years ago

@pjeutr the funny thing is, I can't reproduce it on my computer. The layout looks just fine to me, even if I resize the browser window (linux+ firefox/chrome).

The plunker demo does display the autoscaling problem, though. Any way to force it?

pjeutr commented 7 years ago

@wimrijnders Thanks for looking into this. Just to be clear this is what I see on osx ( firefox / safari / chrome ) The red border is the size of the VisJS canvas.

What I'm looking for is something like this. In this case achieved by height:"350px" because height:"100%" is not working for me.

wimrijnders commented 7 years ago

Yeah, my bad. I neglected to copy the CSS from plunker. I see it now as well.

Sorry to correct you, but you are using Chart.js in the second panel, not GraphJs. it had me confused for a while.

pjeutr commented 7 years ago

Sorry! Must have been because I use Graph as model name. Never heard of GraphJS before.

wimrijnders commented 7 years ago

OK, I can see it happening, so: Confirmed.

It's actually useful that you added the other module; since they are doing something right, I might be able to peek in their code and find out how they fix it. I'll keep you up to date.

wimrijnders commented 7 years ago

Here is where the magic happens. I think. Will verify.

For comparison, this is what Network does.

wimrijnders commented 7 years ago

This is essentially the problem. We need to ensure that the defaults for the canvas size, 300x150 px, do not get used. Canvas.js deals with it, Network must deal with it also.

pjeutr commented 7 years ago

Wow, w3c? Good find! I grepped for 150 in de vis code and couldn't find it 😄

wimrijnders commented 7 years ago

Hehe, now we know where that 150 comes from. This headache will be solved soon.

abcbaby commented 7 years ago

I had this issue awhile back and here is how it resolved it. By default vis.js network height is 100% which is correct. However the default height for canvas, like @wimrijnders says, is 150px. Since the canvas is inside the 'mynetwork' div (default height: auto) tag, it's can only expand as big as the canvas, which is 150px height. In order to adjust to 100% of what YOU want, you have to calculate and adjust the height of the 'mynetwork' div tag, then the canvas can fully resize to whatever you want.

Here is a fork of your @pjeutr code, where i adjust the height of the div tag. You can take a look at: http://plnkr.co/edit/Kkz8SZDgv93yPRpRxckZ?p=preview

Hope this helps!

wimrijnders commented 7 years ago

@dragonzone Great, thanks. But where is the fix exactly in the demo? I can't seem to find it.

abcbaby commented 7 years ago

<div id="mynetwork" style="height: 600px;border: 1px solid red;"></div>

height: 600px Note: The div height can be recalculated and readjust after the network is draw, network.redraw().

wimrijnders commented 7 years ago

@dragonzone I understand the logic of what you're saying: the canvas can only fully expand if the outer element (e.g. for Network div 'mynetwork' in the examples) has a defined value for height. Clear, thanks. I suppose the fix you made to the demo just shows that this is what is happening.

So, in order to ensure that the canvas resizes properly, the size of the outer div must be adjusted to a real value, based on its container. This is what the resize code in Chart.js does.

You know, I think there's a good chance that taking over the code from Chart.js is going to work. Excuse me while I hack it in....

wimrijnders commented 7 years ago

I got something working. It's not so bad for a first attempt:

untitled

I discovered the reason for the size difference: the Chart-code maintains the aspect ratio by default. It's keeping the canvas square. So, the width is correct and then it makes the height equal to the width.

It looks like it's resizing nicely:

untitled2

This is the thinnest I can get it:

untitled3

All in all hopeful. Turning off the aspect ratio shouldn't be too hard.

wimrijnders commented 7 years ago

Here is a plunker demo, using an interim build with the Chart resize code.

Note that I removed the div mynetwork from the demo; it was interfering with the size calculation. Instead it uses the parent element next up. So:

          <div class="panel-body myGraphs">
            <div id="mynetwork" style="border: 1px solid red;"></div>

Becomes:

          <div class="panel-body myGraphs" id="mynetwork" >

Please tell me if this is how you imagined it should be working. If possible, could you try some different resize scenario's as well? You are hereby the designated responsive layout expert 😄.

I'm not done yet, the dynamic resize now overrides all other handling of width/height. And I have to make sure that options.width, options.height and Network.setSize() also work as expected.

wimrijnders commented 7 years ago

Before I forget, a fair warning: There is currently a code freeze in preparation of making the next release. Even if I'm working on the fix, it won't make it into this release.

abcbaby commented 7 years ago

@wimrijnders, although the full size of 100% works, however, now u you lose the capability of making them smaller. For example, try to change the options.height to 50% or 200px, It will no longer work.

pjeutr commented 7 years ago

OK going in the right direction, if I remove the min-height in the css. I see a few pixel difference in height. If I use it in my real life scenario, there's a bigger difference in height. I will make another plnk to show that

pjeutr commented 7 years ago

It's a lot better and acceptable in my scenario. But still not following the outer div like chartjs does, see http://plnkr.co/edit/MGIF7Ukmcj2bJvEGVffu?p=preview

Another thing I see is flicker / redraw in my app, with 3 bigger graphs with ~60 nodes and edges. Dunno if this is related...

@wimrijnders Do you have a fork or diff to look to?

wimrijnders commented 7 years ago

@dragonzone I know. I did mention that:

I'm not done yet, the dynamic resize now overrides all other handling of width/height.

wimrijnders commented 7 years ago

@pjeutr In the given demo, if I comment out the cdn voor the vis.js release version and uncomment:

 <!--<script type="text/javascript" src="http://wimrijnders.nl/public_tmp/issue1832/vis.js"></script>-->

It works pretty much as I expect. Am I missing something?

I did notice some flickering and sometimes a lag in the redraw myself, although on the whole these are infrequent and not really a bother. Truth be told, right now I'm just happy I managed to get it this far. You don't happen to have a demo of that, do you?

Do you have a fork or diff to look to?

Now I do! Here you go. The action is happening in Canvas.js.

wimrijnders commented 7 years ago

Actually, with the flickering, I have a good hunch where it comes from. This if-condition in the code appears to be always true, with the resizing done every single time the method is entered. Will of course look at this.

wimrijnders commented 7 years ago

@pjeutr Thanks BTW for taking my request for checking multiple scenario's seriously. The help is hugely appreciated. I'm not capable of thinking up every possible situation.

pjeutr commented 7 years ago

Yeah, sorry about that didn't double check. Everything is working OK!

I updated the http://plnkr.co/edit/MGIF7Ukmcj2bJvEGVffu?p=preview with heavier stuff that couses redraw / flicker on osx safari. Firefox and chrome are ok with it (I thought chrome safari would act the same being both webkit)

wimrijnders commented 7 years ago

OK, great thanks. Although I wouldn't know how to test safari without an OSX machine here. I did find this though: safari 5 for windows on playonlinux - I would be running an Apple browser in a windows emulator on linux. I don't really know how representative that would be.

wimrijnders commented 7 years ago

No flickering whatsoever on linux+ chromium.
Same for linux+firefox.
linux+safari+wine: I can't get this to install. Sorry.

wimrijnders commented 7 years ago

Here is a demo of latest changes, adjusted from the previous demo. This adds the missing option handling and (hopefully) gets rid of the flickering.

Intent of the demo:

Shows that fixed width and height now work
Show that partial percentages (i.e. any % different from 100%) now work
Show that the resize event is fired correctly.

Do you still have the flickering? There is this thing to try, namely set autoResize: false in the options. See the first network. Please check if this reduces the flickering.

pjeutr commented 7 years ago

Great, flickering on safari is still there. Other browsers are OK. autoResize: false fixes it. But... also stops the resizing 😄 The flicker is not too bad (not always, heartbeat frequency) Think I can sell it as; if you want it nice don't use safari.

To test browsers on windows I use virtualbox, here is a script to get osx running on ubuntu https://github.com/geerlingguy/macos-virtualbox-vm But you need to download an osx installer, give me a holler if you need help.

Other option is... buying a mac 😉 In the past I worked on linux and was forced to buy a macbook for ios development. Never looked back, osx always works no tinkering needed. All my development is on linux. I ssh/mosh and fuse mount to linux machines.

wimrijnders commented 7 years ago

To test browsers on windows ... here is a script to get osx running on ubuntu

OMG this is a thing. Someone would have thought of doing that sooner or later.

The itty bitty little computer I'm typing on right now has been converted to dual boot with Win10, with the goal of being able to test there. There's one problem: The partition is too small to contain Win10, and I still haven't figured out how to resume the updates which are blocked because of this. Who'd have thought 80GB is not enough for an OS?

buying a mac

Had a mac. had macs only sequentially for 18 years. And then I didn't want to have one any more. That was when I realized I could get a much better machine for a third of the prize. Which didn't get in the way of me trying to develop something. Not going back.

wimrijnders commented 7 years ago

The flicker is not too bad (not always, heartbeat frequency)

Aha. This is a giveaway of what's happening. Autoresize is refreshing about once per second, so you really are seeing this happening. There must be better solutions. I have a one-liner idea for this...

...also stops the resizing

Meaning, that the networks are still visible on screen but it doesn't resize along? This is something that can be focused on.

pjeutr commented 7 years ago

Correct, networks are still visible. But doesn't resize. Throw in the one-liner and it's fixed!?

Testing MS browser behaviour is great using vm's. MS provides images themselves https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

wimrijnders commented 7 years ago

OK, my initial hunch was incorrect. I've been doing some online snooping with respect to Safari compatibility, it appears to be OK from Safari 6.1 onwards.

The only thing I can think of is that the canvas rendering is slower on safari, causing the redraw to go over a frame. I'm looking for a benchmark to test this. This looks hopeful, but I can't get it to run.

Update: Running now. I think the page got the hint after I furiously kept on tapping F5.

Would you mind running that test on Safari? To see if the canvas rendering time could be an issue.

These are my scores:

The 'itty bitty' machine: CanvasMark Score: 6308 (Firefox 52 on Linux)
The i7 workstation: CanvasMark Score: 13833 (Chrome 59 on Linux)
The Raspberry Pi: just kidding, that is the equivalent of torturing a kitten

Network runs without flickering on the first two. I don't dare to try it on the third.

pjeutr commented 7 years ago

Seems all OK. This is me: MB pro i5

CanvasMark Score: 8658 (Firefox 55 on Mac)
CanvasMark Score: 8695 (Safari 11 on Mac)
CanvasMark Score: 13038 (Chrome 60 on Mac)
CanvasMark Score: 10912 (Chrome 63 on Mac)

iPhone SE

CanvasMark Score: 11641 (Safari 11 on iOS) also shows flickering running the demo

Rasberry Pi (the first) + Raspbian ( kitties like to be challenged! )

CanvasMark Score: 34416 (Chrome 60 on Linux) Not joking. I suppose something is wrong with the test. If the hardware is to slow

wimrijnders commented 7 years ago

So I finally read the instructions on the benchmark page:

The results can only be compared to other browsers running on the same machine - as each machine with different CPU or graphics will produce difference results.

So it's not an absolute measure. The only thing that can be looked at is your results for the different browsers on the MB.

I was hoping for a clear indication that Safari is slower with the rendering. It's clearly slower than chrome, but comparable to firefox, of which you said there is no flicker problem. So I'm inclined to conclude that this test is moot.

For completeness:

i7 workstation: CanvasMark Score: 13954 (Firefox 52 on Linux)
(previous, for comparison): CanvasMark Score: 13833 (Chrome 59 on Linux)
the 'kitten' (RPi 3): CanvasMark Score: 7711 (Chrome 51 on Linux)

Seeing that the i7 was +-60FPS and the RPi +-12FPS, it's safe to say that comparing values between machines is meaningless. Apologies if I have wasted your time.

wimrijnders commented 7 years ago

IMHO, the flickering comes from here:

Canvas.js:

 setOptions(options) {
    if (options !== undefined) {
      let fields = ['width','height','autoResize'];
      util.selectiveDeepExtend(fields,this.options, options);
    }

    if (this.options.autoResize === true) {
      // automatically adapt to a changing size of the browser.
      this._cleanUp();
      this.resizeTimer = setInterval(() => {  // <=== Timer resizing every second!!
        let changed = this.setSize();
        if (changed === true) {
          this.body.emitter.emit("_requestRedraw");
        }
      }, 1000);                               // <=== End timer
      this.resizeFunction = this._onResize.bind(this);
      util.addEventListener(window,'resize',this.resizeFunction);
    }
  }

This thing looks dated to me, more so since addEventListener has nearly universal support, and it's easy to make a fall-back. I'm going to prepare an interim build for you with this disabled, see if it fares better.

New demo. The only thing changed from previous is the reference to vis.js (added version postfix). This should bury the flickering, can you verify?

pjeutr commented 7 years ago

Yup, it works. Also tried it locally by removing the timer. That was the culprit. Not sure why it was there, but it looks brute force.

wimrijnders commented 7 years ago

Yay! That means we're done with the fix.

One thing to do for me: add unit tests. Also, reminder: This will not make it into next release due to code freeze. I might try whining a bit, you never know....

(Anything I missed? Just checking)

pjeutr commented 7 years ago

Nope, thanks! I downloaded your version. I'll be fine for the next decade 😄

wimrijnders commented 7 years ago

Hehe. Decade is like an epoch in the javascript world. I wouldn't count on it.

OK, until next time! I believe you'll get a notification here when the PR has been published.

wimrijnders commented 7 years ago

@pjeutr I'm rounding off the PR here.

Question: Would you mind if I added the last demo you gave me to the examples? This to showcase that the dynamic sizing works. It's pretty complete and visually enticing.

TIA!

pjeutr commented 7 years ago

Sure, No problem. Glad I could contribute.

On 23 Oct 2017, at 08:57, wimrijnders notifications@github.com wrote:

@pjeutr https://github.com/pjeutr I'm rounding off the PR here.

Question: Would you mind if I added the last demo you gave me to the examples? This to showcase that the dynamic sizing works. It's pretty complete and visually enticing.

TIA!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/almende/vis/issues/1832#issuecomment-338564451, or mute the thread https://github.com/notifications/unsubscribe-auth/AAlWgBvQ2ZRd87Uik4RuhZAm8-Q3s-wmks5svDjOgaJpZM4IS6kN.

wimrijnders commented 7 years ago

Thanks for that! I will attribute you in the source code.

glaggia-larus commented 7 years ago

Hi, is there a schedule for when this change will be released?

Was this tested also with bootstrap grid layout (row + col-xx-xx) ?

pjeutr commented 7 years ago

Hi Wim

I just noticed the edge labels are missing, see also the plnkr demo. When using the original min.vis.js they come back.

@glaggia-larus Check the demo, we tested there with col-md-6, it's easy to change the test to your usecase

almende / vis

Network with 100% height set does not render correctly #1832