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 the width calculation of border-box due to browser zoom #3872

Merged
merged 2 commits into from Dec 6, 2017

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Dec 4, 2017

Summary

The pull request mainly has two tasks done:

However the test for the issue isn't working. I used an iframe test to check for the border-box width at html {zoom: 1.1}. Though opening the html file separately recreates the issue, but via the unittest code, it doesn't.

Some help in fixing the unittest will be really helpful.

Checklist

@jsf-clabot
Copy link

jsf-clabot commented Dec 4, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

A few tweaks, then looks good to me. Thanks @SaptakS!

@@ -53,6 +53,11 @@ define( [
div = null;
}

// Rounding up the exact checking match values
function roundPixelMeasures( measure ) {
return Math.round( parseFloat( measure ) ) + "px";
Copy link
Member

Choose a reason for hiding this comment

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

Adding "px" seems pointless, we should just return and compare against the numeric value (saving a few bytes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed "px" and used the numeric values to compare, since it saves a few bytes in both the places as suggested by you.

@@ -53,6 +53,11 @@ define( [
div = null;
}

// Rounding up the exact checking match values
Copy link
Member

Choose a reason for hiding this comment

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

I find this comment confusing. And given the clarity of the function body, it can probably just be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the comment as suggested by you.

<meta charset="utf-8">
<style>
html {
zoom: 1.1
Copy link
Member

Choose a reason for hiding this comment

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

This should have a semicolon for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved ✔️

@SaptakS
Copy link
Contributor Author

SaptakS commented Dec 4, 2017

@gibson042 will fix these. But the problem is the unittest actually doesn't reproduce the error. So we won't be able to confirm in future if this feature is broken. Can you please help in that as well?

@gibson042
Copy link
Member

The unit test worked for me in Chromium 62 on Linux... assertion failed with the old code and passed with the changes.

Signed-off-by: Saptak Sengupta <saptak013@gmail.com>
…zoom

Signed-off-by: Saptak Sengupta <saptak013@gmail.com>
@SaptakS
Copy link
Contributor Author

SaptakS commented Dec 5, 2017

@gibson042 made the changes as suggested by you.

@gibson042 gibson042 merged commit f00a075 into jquery:master Dec 6, 2017
@veered
Copy link

veered commented Dec 6, 2017

When will this change be published to NPM?

@gibson042
Copy link
Member

Later this month, as part of jQuery 3.3.

@nyurik
Copy link

nyurik commented Dec 16, 2017

Hi, is there any hope for backporting it to the older versions?

@dmethvin
Copy link
Member

You could certainly try to back-port it. We're not supporting older versions so if you want a supported version you should upgrade.

@nyurik
Copy link

nyurik commented Dec 16, 2017

@dmethvin is there any maintenance for the v2.x ? Of course updating to the latest version is preferred, but in some cases some projects are simply too large to port overnight, when some minor issue like this one is discovered.

@dmethvin
Copy link
Member

You're always welcome to port over a "minor issue" for your own use. There's more work than you might think to maintain an old version, especially when it comes to test infrastructure.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants