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

fix(window): apply dpi aware viewport #20522

Closed
wants to merge 1 commit into from

Conversation

@kwonoj
Copy link
Contributor

kwonoj commented Apr 4, 2018

Closes #20505.

This PR is mostly open for discussion, I feel PR may need to be shaped with better organization.

In PR #20228, window::get_coordinates() migrates coordinate calculation includes inner frame viewport
https://github.com/servo/servo/blob/master/ports/servo/glutin_app/window.rs#L700-L701

which was located here previously

fn framebuffer_size(&self) -> DeviceUintSize {
(self.inner_size.get().to_f32() * self.hidpi_factor()).to_u32()
}

It have one differences between, while previous logic was using window::inner_size property while current logic asks to get inner size via get_inner_size(). Those 2 value can be different when dpi_factor is not 1, cause when setting value to inner_size property it calculates back using dpi_factor.

https://github.com/servo/servo/blob/master/ports/servo/glutin_app/window.rs#L546

So previous logic scale via dpi_factor was get correct viewport size, while current logic doubles up its scale. Currently this PR simply looks for places where it asks for get_inner_size() and doesn't apply dpi factor.

Still, it feels somewhat confusing / inconsistent between stored property is differ to what window reports back. Maybe either rename inner_size for more clear (like virtual_inner?), or just store inner_size from get_inner_size() as is and calculate where it's needed, or even could just get rid of inner_size and rely on get_coordinates only.


  • ./mach build -d does not report any errors
  • ./mach build-geckolib does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20505 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____
  • manually verified on windows machine, 150% dpi scaling.

This change is Reviewable

@highfive
Copy link

highfive commented Apr 4, 2018

Heads up! This PR modifies the following files:

@paulrouget
Copy link
Contributor

paulrouget commented Apr 4, 2018

get_inner_size is a winit method. Sadly, the winit coordinates are not typed, so we never know if we get Device pixels or DeviceIndependant pixels.

According to your fix, get_inner_size returns Device pixels. Wondering why it works on Mac then.

@kwonoj
Copy link
Contributor Author

kwonoj commented Apr 4, 2018

Wondering why it works on Mac then

I agree, there seems some different behavior between platform. (So far when I tried on macOS, this didn't happen).

What would be legit way in servo side with current winit's behavior?

@paulrouget
Copy link
Contributor

paulrouget commented Apr 4, 2018

First we need to figure out if winit is indeed inconsistent or not. If it is, we need to fix winit.
Also what's suspicious is the fact that Servo has 2 code path to get the pixel ratio (platform_hidpi_factor). Can't remember why.

@kwonoj
Copy link
Contributor Author

kwonoj commented Apr 4, 2018

@paulrouget

what about part servo stores inner_size as get_inner_size() / dpi_factor? is it desired behavior and expect get_inner_size() should return consistent value?

that Servo has 2 code path to get the pixel ratio (platform_hidpi_factor). Can't remember why.

guess cause winit didn't implement win32 supports? (inlined to return 1.0) https://github.com/tomaka/winit/blob/b40b14f37f0e9ac2c8a525908461d19d0f97cd9e/src/platform/windows/window.rs#L300-L302

@kwonoj
Copy link
Contributor Author

kwonoj commented Apr 4, 2018

looking through, there is upstream PR on stale rust-windowing/winit#332 to get better DPI support on win32 - the situation may get better (or could improve based on those) once it's checked in.

I don't think this PR is acceptable to be checked in as-is, and per suggestion it may better to investigate upstream behavior first before think about servo side workaround - I'm closing PR for now, and leave issue as opened for further tracking.

@paulrouget appreciate for feedback.

@kwonoj kwonoj closed this Apr 4, 2018
@kwonoj kwonoj deleted the kwonoj:invalidate-inner-layout branch Apr 4, 2018
@paulrouget
Copy link
Contributor

paulrouget commented Apr 4, 2018

If we use get_inner_size() / dpi_factor that won't work on Mac as get_inner_size returns "virtual" pixels.

There's also this suspicious behavior where layout is wrong, but click are right.

@kwonoj
Copy link
Contributor Author

kwonoj commented Apr 4, 2018

There's also this suspicious behavior where layout is wrong, but click are right.

Yes, this seems only affect for layout viewport, while input coordinates are correctly calculated.

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

Successfully merging this pull request may close these issues.

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