search

mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.21k stars 35.35k forks source link

Examples get scrollbars when resizing the window and custom scaling in the os. #19136

Open greggman opened 4 years ago

greggman commented 4 years ago
Description of the problem

II noticed today all the examples currently get scrollbars as I resize them on Windows. Depending on the size they appear and disappear.

Chrome 80

コメント 2020-04-15 133410

Firefox 75

コメント 2020-04-15 133452

I'd like to suggest again that I think the default CSS for the examples should effectively be

html,
body,
canvas {
  margin: 0;
  width: 100%;
  height: 100%;
  display: block;
}

or if you want it separated

html {
  height: 100%;
}
body {
  margin: 0;
  height: 100%;
}
canvas {
  width: 100%;
  height: 100%;
  display: block;
}

And I'd suggst at a minimum the example code should be changed to

  1. not have a container. AFAICT it has no point

    container = document.createElement( 'div' );
document.body.appendChild( container );
...
container.appendChild( renderer.domElement );

    to just

    document.body.appendChild( renderer.domElement );

    3 lines to 1

  2. pass false to setSize

        renderer.setSize( window.innerWidth, window.innerHeight, false );

Of course I'd still suggest switching the default to do this all deeper in three and just make CSS the sizing the default and mostly get rid of the need for setSize in most apps as somewhat suggested in #4903

In any case how you fix it or if you fix it is up to you. I just assumed you didn't want scrollbars to appear.

Three.js version
Browser
OS
Hardware Requirements (graphics card, VR Device, ...)
greggman commented 4 years ago

note: to repo this issue you might need to set a custom scaling in Windows

Display Settings->Advanced scaling settings->Custom scaling set to 125

This makes the devicePixelRatio = 1.25 which probably means the actual width of a window is not innerWidth since innerWidth is an integer

Even people with bad eyesite should not have to suffer scrollbars 😛

looeee commented 4 years ago

Can confirm. I didn't check the suggested CSS changes but if they fix this issue then we should make the change. Accessibility is important.

Even people with bad eyesite should not have to suffer scrollbars 😛

Absolutely.

And I'd suggst at a minimum the example code should be changed to not have a container. AFAICT it has no point

I don't think this has anything to do with this issue. Best to discuss one thing at a time.

greggman commented 4 years ago

I don't think this has anything to do with this issue. Best to discuss one thing at a time.

How is discussing solutions not part of fixing it?

looeee commented 4 years ago

My apologies, maybe I misunderstood. Is removing the container required to remove the scrollbars?

Mugen87 commented 4 years ago

II noticed today all the examples currently get scrollbars as I resize them on Windows.

Just for the record: This does not seem to happen on macOS.

gkjohnson commented 4 years ago

I've seen this on my Windows 10 Surface Laptop, too. For some more context we discussed this in https://github.com/mrdoob/three.js/issues/16523#issuecomment-522882779, as well.

Just for the record: This does not seem to happen on macOS.

I actually do see it on my mac some times when zooming the page in but it's weirdly intermittent. Sometimes after resizing the window at 100% scale and then zooming in the scrollbars show up while other times they don't. Here's my mac info:

greggman commented 4 years ago

My point in making the suggested fix is that if the goal is for the canvas to cover the page/window then it's better to ask the browser to do this (which the suggested CSS does above) rather than guess what you think the browser needs (which is what the examples do now and get wrong).

You can keep the container if you want it's just ends up being more code and more css. less code and less css seems simpler and simpler seems to be a goal for three.js so I suggested removing the container. I believe it's there mostly because canvas defaults to display: inline and the container fixed that but setting canvas to display:block removes the need for the container AFAICT.

looeee commented 4 years ago

You can keep the container if you want

I don't have a strong opinion either way on the container unless there's some reason for using it that I'm not aware of.

My reason for asking about it was to clarify whether fixing this means changing a couple of lines in the single CSS file or changing all of the hundreds of example files to remove the container.

greggman commented 4 years ago

My reason for asking about it was to clarify whether fixing this means changing a couple of lines in the single CSS file or changing all of the hundreds of example files to remove the container.

Good point. You can fix it by changing the CSS but IMO it's not a good fix. The issue is three is setting the CSS on the canvas directly via setSize so the only way to fix that with just an edit to main.css AFAICT is

  1. set body to overflow: hidden

    That seems like a hack. It's basically hiding the issue instead of fixing the issue. It will also break the couple of examples that are scrollable.

  2. set html and body to width: 100%; height: 100% and set canvas to width: 100% !imporant; height: 100% !important;

    The !important overrides the the setSize issue which also seems like hiding the issue instead of fixing the issue.

So yes, I believe it requires changing all the examples. Further, the exmaples are all over the map whether they (a) create a canvas and insert in the body (b) create a div insert the canvas in the div and insert the div in the body (c) look up an id="container" element and insert the canvas in that.

I don't think it's possible to fix all of those situations via generic CSS in main.css since the container also needs a width: 100%; height: 100% to tell the browser "cover the page with this" and in each of those sitatuations the container is different.

So, my suggestion is to unify those to either all do the same thing or each fix each one individually to do what's appropriate for that example's current setup. I'd personally choose unify.

looeee commented 4 years ago

Further, the exmaples are all over the map whether they (a) create a canvas and insert in the body (b) create a div insert the canvas in the div and insert the div in the body (c) look up an id="container" element and insert the canvas in that.

That's a fact. The examples were created over many years by many different people and it shows. It would be nice at some point to do a project of unifying and improving code style across all the examples. However, I'd rather wait until the migration to ES6 style is finished before tackling that.

EDIT: although if we decide to remove container/unify container usage we could do that now.

set body to overflow: hidden That seems like a hack

Not sure I agree with you here. That seems like the exact purpose of overflow: hidden. I've never come across anyone saying this should be avoided before.

set canvas to width: 100% !imporant;

If we have to resort to using !important in such a simple CSS file then we are definitely doing something wrong.

greggman commented 4 years ago

set body to overflow: hidden That seems like a hack

Not sure I agree with you here. That seems like the exact purpose of overflow: hidden. I've never come across anyone saying this should be avoided before.

Telling the browser to make something that requires scrollbars and then papering over that issue by hiding the the error is the very defintion of bad programming. That should not be a controversial statement.

If you don't want scrollbars don't ask for content that requires scrollbars in the first place. The only real point of overflow: hidden for when you're doing something that requires your content to go outside like for example 2D HTML labels on a scrolling map.

looeee commented 4 years ago

I'm able to reproduce this with normal scaling at certain browser zoom levels while leaving the example full screen, for example, 125% and 250% in Chrome and Firefox.

This occurs when the container (or body) element ends up with a non-integer size. For example, if the body is 638.666...px wide, window.clientWidth rounds to 639, resulting in a horizontal scrollbar.

@greggman is this what's causing the scrollbars at higher scaling?

mrdoob commented 4 years ago

Looks like changing canvas to display: block introduced this issue. It was all fine when we were using overflow: hidden. I'm partly kidding. I know we were "hiding" the issue 😛

We should definitely do things "the right way" as much as we can, but I was always unsure (and still am) of exposing users to css.

mrdoob commented 3 years ago

Forgot to share here some investigation I did on this issue some months ago: https://github.com/w3c/csswg-drafts/issues/5260#issuecomment-726044922