track the fact that a canvas is provided so we don't overwrite its size #1322

enjalot · 2016-04-01T20:07:53Z

Description:
This attempts to fix #1289 by keeping track of whether a canvas element was provided, and if so don't override it's width and height styles during resize.

Changes proposed:

adds a boolean to the canvas element that the scene will check for in the resize function.

I've made a new example including these changes in a aframe.min.js dist (ran npm run dist on my branch) which shows it seems to work:
http://blockbuilder.org/enjalot/dfb4b1220ba76b63b8fa2ed2c5d517c3

cvan · 2016-04-01T20:09:48Z

src/core/scene/a-scene.js


        // Update canvas.
-        if (!isMobile) {
+        if (!isMobile && !canvas.provided) {


this assumes that those providing their own canvas are handling resizing themselves - which is probably a fair assumption

yes, my personal use case is that i'd like to be able to dedicate some subset of the page to my aframe scene and have other html surrounding it. I would be assuming control over how much of the page my canvas occupies.

You have canvas.a-canvas class available

Though that is not specific, enough we could set <canvas data-aframe-provided> on the provided canvas

yeah, it should be a data attribute IMO. I've seen non-browser properties get unset; tbh I don't know how browsers always handle those.

enjalot · 2016-04-01T20:43:26Z

@cvan @ngokevin i've updated the code to use data attributes, is this what you had in mind?

cvan · 2016-04-01T20:44:59Z

src/components/scene/canvas.js

      canvas.style.width = data.width + '%';
      scene.appendChild(canvas);
+    } else {
+      canvas.dataset.aframeProvided = true;


this shouldn't be in the else

i didn't fully understand the architecture. is everything that happens in this component the result of a provided canvas?

You should move it up to the if statement where the provided canvas is created.

Ah, I think the confusion is we're thinking "provided by A-Frame", and you're thinking "provided by user"

ooo, I'll switch it to that then.

enjalot · 2016-04-01T21:09:23Z

I've also created a boilerplate-canvas demo which is a simple adaptation of the boilerplate-helloworld demoing a user provided canvas. would you like me to include that?

ngokevin · 2016-04-01T21:12:14Z

src/core/scene/a-scene.js


        // Update canvas.
-        if (!isMobile) {
+        if (!isMobile && canvas.dataset.aframeProvided) {


I'd update the comment Update canvas if canvas was provided by A-Frame.

ngokevin · 2016-04-01T21:13:59Z

Yeah, maybe let's include that as a start for embedded/inline scenes? That's one of our goals:

like: boilerplate-embedded-scenes

…nvas

cvan · 2016-04-01T21:31:59Z

examples/boilerplate-embedded-scenes/index.html

+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Hello, World! • A-Frame</title>


perhaps change title to mention Embedded?

ngokevin · 2016-04-01T21:33:44Z

examples/boilerplate-embedded-scenes/index.html

+  </head>
+  <body>
+    <p>Through the looking glass.</p>
+    <canvas id="mycanvas"></canvas>


Maybe move the canvas into the scene element?

I think it's fine to show that that's not necessary

I'd encourage it to wrap everything up nicely.

enjalot · 2016-04-02T02:37:54Z

btw I left the canvas element outside of the scene (with a comment) because to me that's one of the main motivations of this feature. I want to be able to declare my scene decoupled from where it's laid out in the page.

dmarcos · 2016-04-02T03:16:11Z

src/components/scene/canvas.js

      canvas.style.height = data.height + '%';
      canvas.style.width = data.width + '%';
+      // Mark canvas as provided/injected by A-Frame.
+      canvas.dataset.aframeProvided = true;


In the light and camera systems we mark the default elements like this:

ambientLight.setAttribute('data-aframe-default-light', ''); cameraWrapperEl.setAttribute('data-aframe-default-camera', '');

I think we should either change this or change the camera and light systems to follow this convention. @ngokevin what do you think?

I would say we also add the aframeProvided data attribute to the lights/camera (#798), but need to keep the default-light/default-camera so they can be removed individually

it's not clear to me if I should make a change here.

No action required.

Can we just changed the name to aframeDefault? We can merge after that

cvan · 2016-04-03T20:22:20Z

examples/boilerplate-embedded-scenes/index.html

+
+    <script>
+      var scene = document.querySelector('a-scene');
+      function check () {


don't want you want to call check on resize of window?

I'm adding it, just to confirm that aframe's resize logic doesn't overwrite the user provided canvas dimensions.

cvan · 2016-04-12T08:26:27Z

@enjalot this is almost ready - let us know when you've made the minor changes above. thanks! 😄

enjalot · 2016-04-15T02:46:39Z

@cvan i think i've addressed all the concerns. let me know what else i can do

ngokevin · 2016-04-15T19:19:58Z

Patch looks good. Still a bit of work to go for better support for embedded scenes, but this is a big step:

@dmarcos look good?

dmarcos · 2016-04-19T04:01:13Z

@enjalot Thanks!

enjalot · 2016-04-19T17:01:33Z

woot!

ngokevin · 2016-04-19T19:07:02Z

nice!

@dmarcos for future, you can use the github squash merge, the green button has a dropdown now

dmarcos · 2016-04-19T19:09:46Z

@ngokevin I know the button but I don't always check that there's a single commit on the PR.

track the fact that a canvas is provided so we don't overwrite it's size

1cb1725

cvan reviewed Apr 1, 2016
View reviewed changes

use data attributed to track whether canvas is provided

90f343f

cvan reviewed Apr 1, 2016
View reviewed changes

switch canvas provided boolean to mean a-frame provided it

78fdc3e

ngokevin reviewed Apr 1, 2016
View reviewed changes

enjalot added 2 commits April 1, 2016 14:21

add comments around provided canvas tracking

c44ac63

first draft of an embedded scenes example, utilizing user-provided ca…

154452a

…nvas

cvan reviewed Apr 1, 2016
View reviewed changes

cvan changed the title ~~track the fact that a canvas is provided so we don't overwrite it's size~~ Apr 1, 2016

ngokevin reviewed Apr 1, 2016
View reviewed changes

cvan added the needs changes label Apr 2, 2016

code style cleanup

aab0734

dmarcos reviewed Apr 2, 2016
View reviewed changes

cvan reviewed Apr 3, 2016
View reviewed changes

clean up embedded scene example

7a82e7d

ngokevin added needs review and removed needs changes labels Apr 15, 2016

dmarcos added needs changes and removed needs review labels Apr 15, 2016

change aframeProvided -> aframeDefault

d42ce03

dmarcos merged commit 48691d3 into aframevr:master Apr 19, 2016

ngokevin mentioned this pull request May 16, 2016

Display video inline without an iframe #1449

Closed

Uh oh!

track the fact that a canvas is provided so we don't overwrite its size #1322

track the fact that a canvas is provided so we don't overwrite its size #1322

Uh oh!

Conversation

enjalot commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enjalot commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enjalot commented Apr 1, 2016

Choose a reason for hiding this comment

ngokevin commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enjalot commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan commented Apr 12, 2016

enjalot commented Apr 15, 2016

ngokevin commented Apr 15, 2016

dmarcos commented Apr 19, 2016

enjalot commented Apr 19, 2016

ngokevin commented Apr 19, 2016

dmarcos commented Apr 19, 2016

Labels

4 participants