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

Handle scale factor change on web-sys backend #1690

Merged

Conversation

alvinhochun
Copy link
Contributor

@alvinhochun alvinhochun commented Aug 29, 2020

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

This implements scale factor change detection for the web target by using MediaQueryList. I've only implemented this for the web-sys backend. It's currently stubbed out in the stdweb backend.

This PR also includes a small refactor and fixes an issue with the handling of ControlFlow::Exit (see also #1688).

Tested on Firefox and Chrome (Windows) And Firefox (Linux).

Copy link
Contributor

@ryanisaacg ryanisaacg left a comment

Choose a reason for hiding this comment

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

Nice work, just a couple things before we're good to go 👍

FEATURES.md Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ryanisaacg ryanisaacg left a comment

Choose a reason for hiding this comment

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

👍

}

fn handler(&mut self, event: MediaQueryListEvent) {
assert_eq!(event.matches(), false);
Copy link
Contributor

@Maximetinu Maximetinu Dec 20, 2022

Choose a reason for hiding this comment

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

hey @alvinhochun, can I ask what the reason was for this assertion? We are hitting it a lot on a random basis when re-scaling the browser window, but it's erratic and only happens to a few people. We haven't figured out the root cause yet 😬

Is this something that could be changed by a debug assert instead? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, looking back now I don't really see why this assert would fail, but removing this assert should be perfectly fine because it is recoverable. If you think this is appropriate you may change it to just print a debug message somewhere on debug builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! In that case, I'm going to dig a bit more and maybe open a PR for that. 🙂 Thanks for the quick response!

What do you think that message should look like? I still don't quite get what this matches call to the media query is doing.

I know it relates to this https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryListEvent/matches but I don't see why does it matter here whether .matches() return true or false, or why it should always return false in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants