Skip to content
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

Can't move camera in babylon demos #24363

Closed
jdm opened this issue Oct 3, 2019 · 12 comments
Closed

Can't move camera in babylon demos #24363

jdm opened this issue Oct 3, 2019 · 12 comments
Labels

Comments

@jdm
Copy link
Member

@jdm jdm commented Oct 3, 2019

[2019-10-03T19:24:15Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:234 setting getter-only property "buttons"
[2019-10-03T19:24:15Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:15Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:15Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:16Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:16Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:16Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:16Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:17Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:153:28 setting getter-only property "buttons"
[2019-10-03T19:24:26Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:26Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:26Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:29Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
[2019-10-03T19:24:45Z ERROR script::dom::bindings::error] Error at https://code.jquery.com/pep/0.4.2/pep.min.js:149:48 setting getter-only property "buttons"
@jdm jdm added the A-content/dom label Oct 3, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Oct 3, 2019

This comes from this code:

prepareButtonsForMove:function(a,b){var c=H.get(this.POINTER_ID);
// Update buttons state after possible out-of-document mouseup.
0!==b.which&&c?a.buttons=c.buttons:a.buttons=0,b.buttons=a.buttons}

I believe b comes from calling this function:

prepareEvent:function(a){var b=u.cloneEvent(a),c=b.preventDefault;return b.preventDefault=function(){a.preventDefault(),c()},b.pointerId=this.POINTER_ID,b.isPrimary=!0,b.pointerType=this.POINTER_TYPE,b}

And then there's:

     * Returns a snapshot of inEvent, with writable properties.
     *
     * @param {Event} inEvent An event that contains properties to copy.
     * @return {Object} An object containing shallow copies of `inEvent`'s
     *    properties.
     */
cloneEvent:function(a){for(var b,c=Object.create(null),d=0;d<q.length;d++)b=q[d],c[b]=a[b]||r[d],
// Work around SVGInstanceElement shadow tree
// Return the <use> element that is represented by the instance for Safari, Chrome, IE.
// This is the behavior implemented by Firefox.
!t||"target"!==b&&"relatedTarget"!==b||c[b]instanceof SVGElementInstance&&(c[b]=c[b].correspondingUseElement);
// keep the semantics of preventDefault
return a.preventDefault&&(c.preventDefault=function(){a.preventDefault()}),c}

So something about cloneEvent is causing the writable properties not to work right.

@jdm
Copy link
Member Author

@jdm jdm commented Oct 3, 2019

var q=[
// MouseEvent
"bubbles","cancelable","view","detail","screenX","screenY","clientX","clientY","ctrlKey","altKey","shiftKey","metaKey","button","relatedTarget",
// DOM Level 3
"buttons",
// PointerEvent
"pointerId","width","height","pressure","tiltX","tiltY","pointerType","hwTimestamp","isPrimary",
// event instance
"type","target","currentTarget","which","pageX","pageY","timeStamp"],r=[
// MouseEvent
!1,!1,null,null,0,0,0,0,!1,!1,!1,!1,0,null,
// DOM Level 3
0,
// PointerEvent
0,0,0,0,0,0,"",0,!1,
// event instance
"",null,null,0,0,0,0]
t="undefined"!=typeof SVGElementInstance
@jdm
Copy link
Member Author

@jdm jdm commented Oct 3, 2019

Adding SVGElementInstance through a userscript did not make a difference.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 9, 2019

Which demo?

@jdm
Copy link
Member Author

@jdm jdm commented Oct 9, 2019

Any of the ones that I've tried on desktop.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 9, 2019

Oh, not in immersive mode right?

@jdm
Copy link
Member Author

@jdm jdm commented Oct 9, 2019

Correct.

@jdm
Copy link
Member Author

@jdm jdm commented Oct 18, 2019

This now works as expected. I wonder if #24409 fixed it?

@jdm jdm closed this Oct 18, 2019
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 28, 2019

This now works as expected. I wonder if #24409 fixed it?

Nope. I changed the bbjs code to not fail.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 28, 2019

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 28, 2019

This is actually the proper behavior when using use strict; + event.buttons = ....

The problem is that this jquery test fails when it should not: https://github.com/jquery/PEP/blob/e880e9d26c7a45cab153b8bf2e03fc51b64844bf/src/mouse.js#L13

The buttons property is introduced here where it is set to 0 not matter what comes with the constructor: be3cb00#diff-bc92fc3e149d38d401fb237b255cab2cR133

PR incoming.

@paulrouget paulrouget mentioned this issue Oct 28, 2019
4 of 5 tasks complete
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 28, 2019

With #24564, no errors, and the camera moves as expected.

bors-servo added a commit that referenced this issue Oct 28, 2019
Properly set MouseEvent buttons property

Properly set MouseEvent buttons property. Same behavior as Chrome and Firefox.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24363 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Oct 28, 2019
Properly set MouseEvent buttons property

Properly set MouseEvent buttons property. Same behavior as Chrome and Firefox.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24363 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Oct 28, 2019
Properly set MouseEvent buttons property

Properly set MouseEvent buttons property. Same behavior as Chrome and Firefox.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24363 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Oct 28, 2019
Properly set MouseEvent buttons property

Properly set MouseEvent buttons property. Same behavior as Chrome and Firefox.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24363 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Oct 29, 2019
Properly set MouseEvent buttons property

Properly set MouseEvent buttons property. Same behavior as Chrome and Firefox.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24363 (GitHub issue number if applicable)

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.