Skip to content

Fix ScaleManager reference to the parent element#467

Closed
pantoninho wants to merge 1 commit intophaserjs:masterfrom
pantoninho:master
Closed

Fix ScaleManager reference to the parent element#467
pantoninho wants to merge 1 commit intophaserjs:masterfrom
pantoninho:master

Conversation

@pantoninho
Copy link

@pantoninho pantoninho commented Feb 9, 2018

This PR

  • is a bug fix

Live Example: https://codepen.io/pantoninho/pen/rJyjxb

Describe the changes below:

ScaleManager references this.boundingParent to calculate the initial canvas size, but this property does not hold any value.. I suppose it was left-over code from previous commits.

There were multiple reports in the original phaser repo that may be related to this issue:
phaserjs/phaser#2592
phaserjs/phaser#2458
phaserjs/phaser#2556

Replacing this.boundingParent references to this.parentNode solves the problem and everything works as expected.

The grunt task is not re-building the custom builds, is it supposed to?

@samme
Copy link
Collaborator

samme commented Feb 10, 2018

ScaleManager#boundingParent should equal null in certain cases (but never undefined).

@samme
Copy link
Collaborator

samme commented Feb 10, 2018

The problem is that game.canvas is not yet defined at that point.

So boundingParent and getParentBounds() are actually correct, but setupScale() isn't correct to call it that way.

We need to keep boundingParent in getParentBounds() because that's the correct behavior, but we could add a second argument, parentNode, to let setupScale() override that value.

@samme
Copy link
Collaborator

samme commented Feb 10, 2018

@samme samme closed this in 4500799 Feb 11, 2018
@samme samme added this to the 2.10.1 milestone Feb 11, 2018
@samme
Copy link
Collaborator

samme commented Feb 11, 2018

@pantoninho most of them don't, I think. See grunt --help for the task list.

The grunt task is not re-building the custom builds, is it supposed to?

@pantoninho
Copy link
Author

pantoninho commented Feb 11, 2018

I'm not sure if I understood what you've said @samme, but I've found no references to the scale manager's boundingParent throughout the project. Is that property still necessary?

If yes, I'd like some more information on how to fix this issue this correctly.
Thank you!

@samme
Copy link
Collaborator

samme commented Feb 11, 2018

https://github.com/photonstorm/phaser-ce/blob/v2.10.0/src/core/ScaleManager.js#L2204

4500799 should fix this.

samme added a commit that referenced this pull request Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants