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.

Sign up for GitHub

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

Jump to bottom

feature: Detect parent resize #1365

Merged
merged 3 commits into from
Sep 3, 2021
Merged

feature: Detect parent resize #1365

merged 3 commits into from
Sep 3, 2021

Conversation

baleeds
Copy link
Contributor

@baleeds baleeds commented Feb 7, 2020

Fixes an issue where if the screen div resizes for a reason other than window resize, the canvas wouldn't redraw.

#1364

@baleeds baleeds mentioned this pull request Feb 7, 2020
Closed
@baleeds
Copy link
Contributor Author

baleeds commented Feb 7, 2020

It appears that TravisCI doesn't install dependencies before running tests?

@CendioOssman
Copy link
Member

Yeah, we don't want any external dependencies for the project. So please put a copy of what you need in the vendor/ directory. We also want things to be ES6 modules, so some tweaking might be required.

@samhed samhed added the feature label Feb 10, 2020
samhed
samhed requested changes Feb 10, 2020
View reviewed changes
Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, thank you for this PR.

A few comments on the code, and two general points:

  • Could you run 'npm run lint' and fix the whitespace issues?
  • Remove the 'yarn.lock' please

core/rfb.js Outdated Show resolved Hide resolved
core/rfb.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@samhed samhed added this to the v1.2.0 milestone Feb 10, 2020
@samhed samhed linked an issue Feb 10, 2020 that may be closed by this pull request
Detect parent element resize to redraw canvas (instead of window.onresize) #1364
Closed
samhed
samhed requested changes Feb 12, 2020
View reviewed changes
Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are loads of unrelated whitespace changes to core/rfb.js, please remove these from the PR.

Also, could you update LICENSE.txt?

After that I think we'll be getting close to merging this, I'll try to find time for some testing soon as well.

samhed
samhed requested changes Feb 12, 2020
View reviewed changes
LICENSE.txt Outdated Show resolved Hide resolved
@CendioOssman
Copy link
Member

This looks like it is pretty much ready to be merged. What browsers have been tested?

@baleeds
Copy link
Contributor Author

baleeds commented Feb 28, 2020

This looks like it is pretty much ready to be merged. What browsers have been tested?

I've tested in Chrome, Firefox, and Safari. I haven't looked at IE or Edge, but according to the polyfill docs, IE 11 is supported, along with all versions of Edge.

samhed
samhed previously approved these changes Mar 1, 2020
View reviewed changes
Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks

@samhed
Copy link
Member

samhed commented Mar 1, 2020

I'll help out with some testing on IE and Edge tomorrow.

@baleeds
Copy link
Contributor Author

baleeds commented Mar 27, 2020

Hi @samhed, did you get a chance to test on IE?

@samhed
Copy link
Member

samhed commented Mar 28, 2020

Shit.. sorry @baleeds, I totally forgot about this.

When I checkout your branch I can't even get things going however. First I get:

core/rfb.js:23 /vendor/resize-observer-polyfill/dist/ResizeObserver net::ERR_ABORTED 404 (File not found)

If I add ".js" to the filename in the import I get the following error:

SyntaxError: The requested module '../vendor/resize-observer-polyfill/dist/ResizeObserver.js' does not provide an export named 'default'

@samhed samhed dismissed their stale review March 28, 2020 20:59

Page doesn't load

samhed
samhed requested changes Mar 28, 2020
View reviewed changes
Copy link
Member

@samhed samhed left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need ES6 modules, but it looks like you copied the folder from node_modules?

@samhed
Copy link
Member

samhed commented May 16, 2020

Ping @baleeds

@baleeds
Copy link
Contributor Author

baleeds commented May 18, 2020

Hi sorry! I don't know how to do it differently. Is there a process that I should be taking for importing the library as an ES6 module?

@CendioOssman
Copy link
Member

Your import right now seems to be geared towards NodeJS. Are you testing this code directly in a browser without any modifiction? I.e. no browserify, babel or anything like it.

First you need to have an ES6 version of the code, which seems to already exist in the form of ResizeObserver.es.js . Then you need to import by filename. Lastly, let's not ship things we don't need to clean out any files that aren't used.

@samhed samhed removed the noresponse label May 31, 2020
@samhed
Copy link
Member

samhed commented Jul 1, 2020

Do you think you will get a chance to fix this @baleeds?

@baleeds
Copy link
Contributor Author

baleeds commented Jul 2, 2020

Do you think you will get a chance to fix this @baleeds?

Hi Samhed, sorry I've been non-responsive. Our project went in a different direction in terms of technology. I do think this change is worthwhile, but I don't have time to look at it now. It was only a few lines of code, and then adding in the library, if you wanted to make it happen.

@CendioOssman CendioOssman removed this from the v1.2.0 milestone Jul 3, 2020
@CendioOssman
Copy link
Member

We've dropped support for Internet Explorer so this should be possible to merge without the polyfill now.

@samhed samhed force-pushed the feature/detect-parent-resize branch from 12cbfb1 to a5bc327 Compare April 14, 2021 09:18
@samhed samhed added this to the v1.3.0 milestone Apr 14, 2021
@samhed samhed removed the noresponse label Apr 14, 2021
@samhed samhed self-assigned this Apr 14, 2021
@samhed
Copy link
Member

samhed commented Apr 14, 2021

I have removed the polyfill, removed unrelated changes and rebased on master. I have manually tested on:

Windows 10

  • Edge 89
  • Chrome 89
  • Firefox 87

macOS 11

  • Safari 14
  • Chrome 89
  • Firefox 86

iOS 14.4

  • Safari

Android 11

  • Chrome 89
  • Firefox 87

What remains is to modify the unit tests, they rely on the resize event at the moment.

Benjamin Leeds and others added 3 commits September 1, 2021 23:45
@samhed
Replace window.onresize with ResizeObserver
a9c2ff3 
Fixes an issue where if the screen div resizes for a reason other than
window resize, the canvas wouldn't redraw.
@samhed
Modify unit tests to work with ResizeObserver
375f36c
@samhed
Increase browser version requirements
1afa18f 
Now that we use ResizeObserver we know that we require more modern
browsers. The most notable ones here are Firefox and Safari.

With regards to Firefox, while the desktop version has had support
since 69, the Android app requires 79. At the time of writing the
current ESR of Firefox is 78, but the concept of ESR doesn't seem to
exist for Android.

The Safari 13 requirement means we no longer support for example iPhone
5S or the 4th generation of the iPad. These are devices from 2013~2014.
@samhed samhed force-pushed the feature/detect-parent-resize branch from a5bc327 to 1afa18f Compare September 3, 2021 15:05
@samhed samhed merged commit 7841037 into novnc:master Sep 3, 2021
@samhed
Copy link
Member

samhed commented Sep 3, 2021

Unit tests have now been added and the change is merged.

@CendioOssman CendioOssman mentioned this pull request Nov 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samhed samhed samhed requested changes

@CendioOssman CendioOssman Awaiting requested review from CendioOssman

Assignees

@samhed samhed

Labels
feature
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Detect parent element resize to redraw canvas (instead of window.onresize)
3 participants
@baleeds @CendioOssman @samhed