/jquery

css() reports wrong width/height in Chrome on certain zoom levels #3808

Closed
bschwarzent opened this Issue Oct 4, 2017 · 12 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  3.3.0
7 participants
@bschwarzent @gibson042 @simonemazzoni @SaptakS @timmywil @cuzzins @mgol
@bschwarzent

bschwarzent commented Oct 4, 2017

jQuery 3.2.1 reports wrong sizes for elements with box-sizing: border-box in Chrome (v61) on certain browser zoom levels. It seems to add the element's insets (padding + border) to its width and height, although the CSS width already includes the insets.

Demonstration

http://output.jsbin.com/yipudeloya/

In the provided test case, we have two <div>s with 10px padding on all sides, 1px border on all sides and a CSS width/height set to 32px. Because box-sizing is set to border-box, the visual size of the elements is 32x32px with a remaining "inner" space of 10px. At 100% browser zoom level, this is correctly reported both by css('width') and .outerWidth().

If we now zoom to 110%, the numbers get slightly changed, probably due to some rounding errors (31.9886px instead of 32px). Because we should always get "CSS pixels" that are not affected by browser zoom, I would have expected those numbers to stay the same - but I can live with that. The real problem occurs when you reload the page while having the browser zoom at 110%! Now the width is completely incorrect (53.977236000000005px instead of 32px).

If the page uses those values for programmatic layout, the results will be wrong. You can see the effect in the demo. The CSS width and height of the first <div> are copied to the second, so they should always be the same size. However, if you start at 100%, the second <div> is much larger.

Analysis

Analysis has shown that this seems to be caused by the internal function computeStyleTests() in css/support.js that wrongly sets the internal property boxSizingReliableVal to false. Apparently, this happens because of a rounding error. In my case, the test div's measured width is 3.997px, which is not equal to the expected value of 4px. What's worse, this internal property is only calculated once! So when you start at 110% zoom, and then reset it to 100%, the problem still persists until you completely reload the page.

I could observe the problem at the following zoom levels: 110%, 90%, 80%, 67% and 33%.

Firefox (v56) and IE (v11) don't seem to be affected by this problem. Also, with jQuery 2.1.4, everything works fine on all browsers. All tests were performed on Windows 7 with default DPI and font size settings.
@gibson042
Member

gibson042 commented Oct 4, 2017

Ugh, confirmed on the git build. I think we need something like a unit-preserving round comparison. Fortunately, this is reproducible with html { zoom: 1.1 }, so we'll be able to verify a fix with unit testing.

@gibson042 gibson042 added CSS Dimensions and removed CSS labels Oct 4, 2017

@simonemazzoni

simonemazzoni commented Oct 11, 2017

Same here, @gibson042 any news about it?
Up to now I fixed downgrading to version 3.1.1.
@gibson042
Member

gibson042 commented Oct 13, 2017

A PR would be welcome, but if nobody expresses interest then I'll probably put in a fix before the next release
@SaptakS
Contributor

SaptakS commented Nov 6, 2017

@gibson042 I would like to work on this issue.
@timmywil
Owner

timmywil commented Nov 6, 2017

@SaptakS Feel free!

hubot pushed a commit to eclipse/scout.rt that referenced this issue Nov 24, 2017

@bschwarzent @cguglielmo
jQuery: add patch for issue #3808 (wrong CSS sizes) 
jquery/jquery#3808

Change-Id: I4b3b9dd60392a8724f4a660c9b7e4d6465762add
Reviewed-on: https://git.eclipse.org/r/112249
Tested-by: Hudson CI
Reviewed-by: Claudio Guglielmo <[email protected]>
778411b

hubot pushed a commit to eclipse/scout.rt that referenced this issue Nov 24, 2017

@bschwarzent @cguglielmo
jQuery: add patch for issue #3808 (wrong CSS sizes) 
jquery/jquery#3808

Change-Id: I4b3b9dd60392a8724f4a660c9b7e4d6465762add
Reviewed-on: https://git.eclipse.org/r/112249
Tested-by: Hudson CI
Reviewed-by: Claudio Guglielmo <[email protected]>
(cherry picked from commit 778411b)
Reviewed-on: https://git.eclipse.org/r/112250
Tested-by: Claudio Guglielmo <[email protected]>
594de0d
@gibson042
Member

gibson042 commented Nov 25, 2017

@SaptakS See https://jsbin.com/tutusofosa/edit?html,css,js,output .

@SaptakS SaptakS referenced this issue Dec 4, 2017

Merged

Fix the width calculation of border-box due to browser zoom #3872

2 of 4 tasks complete

@gibson042 gibson042 closed this in f00a075 Dec 6, 2017

@gibson042 gibson042 added this to the 3.3.0 milestone Dec 6, 2017

@cuzzins

cuzzins commented Dec 6, 2017

Just to add to this, I reported this (or a very similar issue back in May) and was essentially told that browser zoom is not supported and I should essentially expect things to break around it between versions. Has the policy changed?

In the end I had to rip the code out that was using jQuery and rewrite it, and also have not been able to rely on jQuery height/width for other parts as it may break with no intention of fixing it.

See #3681 for what I originally described as the issue
@gibson042
Member

gibson042 commented Dec 6, 2017

You didn't do anything wrong, but this particular issue was phrased in such a way and came with sufficient detail that it merited more attention. Even though both probably stemmed from similar underlying causes, this one highlighted a fundamental problem of our support tests failing with non-default zoom levels, which is more severe than any given response being wrong. And as @dmethvin noted, landing #3738 and #3656 changed things. We still can't guarantee behavior with non-default zoom, but as of f00a075 we have confidence that our browser behavior observations are correct (which is likely to resolve issues like #3681).

Our official policy remains "dimensions may be incorrect when the page is zoomed by the user", but we do try to make things work even outside of normal operating parameters if a fix can be achieved without incurring undue cost (factoring in all dimensions—file size, performance, code complexity, testability, maintenance burden, followup reports, etc.). I feel bad that you had to remove jQuery dimensions calls for your application, and hope that you will consider restoring them after 3.3 is released this month.

And please do report issues you encounter in the future. Our notices are caveats, not absolutes.

@GulajavaMinistudio GulajavaMinistudio referenced this issue in GulajavaMinistudio/jquery Dec 7, 2017

Merged

Update upstream #31

0 of 4 tasks complete
@bschwarzent

bschwarzent commented Dec 12, 2017

@gibson042 Is there a way for me to verify the fix? I tried https://code.jquery.com/jquery-git.js, but that file does not seem to include the fix (yet?).
@gibson042
Member

gibson042 commented Dec 13, 2017

Yes, that's correct. But it looks like the file is stuck: Date: 2017-11-28T12:34Z. @jquery/core, does someone need to kick Jenkins?
@mgol
Member

mgol commented Dec 13, 2017

@gibson042 that's what our Jenkins looks like now most of the time:
screen shot 2017-12-13 at 09 32 52
Content jobs are broken & never end. I'm killing them all manually from time to time but I've been on vacation for the past few days; I've just done that and the jobs should go through.
@bschwarzent

bschwarzent commented Dec 13, 2017

Thanks! I was able to successfully verify the fix with the updated jquery-git.js version. 👍

@nyurik nyurik referenced this issue in elastic/kibana Dec 16, 2017

Closed

Upgrade jquery #11390

restonexyz added a commit to restonexyz/jquery that referenced this issue Feb 3, 2018

@restonexyz
up (#1) 
* Dimensions: ignore transforms when retrieving width/height

Close gh-3561
Fixes gh-3193

* CSS: remove dead code in getWidthOrHeight

- getCSS already falls back to inline styles

Ref gh-3561

* Release: update release dependencies

* Release: update AUTHORS.txt

* Release: update version to 3.2.0-pre

* Release: md5sum -> md5 -r for MAC

* Build: Updating the master version to 3.2.1-pre.

* Release: edit dist README version on release

Fixes gh-3574

* Build: update PR template

- Comment out things we don't need to see in the PR description
- Change CLA link

* Tests: move readywait to an iframe test

Close gh-3576
Fixes gh-3573

* Dimensions: fall back to offsetWidth/Height for inline elems

Close gh-3577
Fixes gh-3571

* Revert "Event: Trigger checkbox and radio click events identically"

This reverts commit b442aba.

* Revert "Event: Add radio click triggering tests"

This reverts commit 5f35b5b.

* Tests: add test for passing trigger data to radio click handler

Close gh-3581
Fixes gh-3579

* Build: Updating the master version to 3.2.2-pre.

* CSS: retrieve inline style before computed

- Fixes an issue with getting computed style on detached elements

* Revert "Build: Updating the master version to 3.2.2-pre."

This reverts commit 066bd86.

* Build: Updating the master version to 3.2.2-pre.

* Tests: Fix incorrect assert name for ensure_iterability_es6

Closes gh-3584
Ref bb026fc.

* Docs: Update links to HTML spec for stripAndCollapse (#3594)

* Offset: Use correct offset parents; include all border/scroll values

Thanks @anseki

Fixes gh-3080
Fixes gh-3107
Closes gh-3096
Closes gh-3487

* Core: Update isFunction to handle unusual-@@toStringTag input

Ref gh-3597
Fixes gh-3600
Fixes gh-3596
Closes gh-3617

* Tests: Improve offset test setup and labels

Hopefully this fixes iOS testing: http://swarm.jquery.org/job/5226

Ref 1d2df77
Closes gh-3641

* Tests: Be even more async for iOS

Ref 1d2df77
Closes gh-3643

* Tests: Attach test iframes to the body for visibility-dependent code

Ref 1d2df77
Closes gh-3645

* Tests: Allow a mock QUnit.test for perfect testIframe fidelity

Ref 1d2df77
Closes gh-3647

* Tests: Prepend test iframes for even *more* consistency

Ref 1d2df77

* Tests: Reset iframe window scroll after updating html/document position

Ref 1d2df77
Closes gh-3649

* Tests: Add debugging to investigate iOS failures

Ref 1d2df77

* Tests: Keep iframes visible in TestSwarm

Ref 1d2df77

* Tests: Adjust by actual scroll position, rather than expected

Ref 1d2df77

* Tests: Clean up offset debugging

Ref 1d2df77
Ref c0edd8d

* Tests: Correct expected assertion count

Ref e94b5b0

* Tests: Revert some testIframe changes to fix dimensions tests

Ref c0edd8d

* Revert "Tests: Revert some testIframe changes to fix dimensions tests"

This reverts commit c4368a9.

* Tests: Revert some testIframe changes to fix dimensions tests

Ref c0edd8d

* CSS: Drop the float mapping from cssProps

Firefox 35 and newer support style.float directly.

Closes #3569

* Docs:Tests: Update IE/Edge-related support comments & tests

Closes gh-3661

* Build: Test on Node.js 8, stop testing on Node.js 7

* Tests: minor typos

Close gh-3671

* Dimensions: Include scroll gutter in "padding" box

Fixes gh-3589
Closes gh-3656

* Deferred: fix memory leak of promise callbacks

Fixes gh-3606
Closes gh-3657

* Build: update node dependencies; commit package-lock.json

- Also ignore yarn.lock
Close gh-3669

* Build: Update sinon, husky, and qunitjs

* Build: fix uglify options for uglify update

- Uses new typeofs option for compression
- See mishoo/UglifyJS2#2198

Close gh-3710

* Event: `stopPropagation()` on native event-handler

Fixes gh-3693
Close gh-3694

* Core: Deprecate jQuery.isWindow

Fixes gh-3629
Close gh-3702

* Test: ensure position/offset return mutable objects

Fixes gh-3612
Closes gh-3695

* Revert "Offset: Resolve strict mode ClientRect "no setter" exception"

This reverts commit 3befe59.

* Offset: fix error from bad merge in #3695

* Dimensions: Detect and account for content-box dimension mishandling

Fixes gh-3699
Closes gh-3700

* Support: Properly check for IE9 absolute scrollbox mishandling

Ref gh-3589
Fixes gh-3699
Fixes gh-3730
Closes gh-3729

* Tests: Try extra hard to control focus

Ref gh-3732

* Tests: Abort focus tests when the environment doesn't cooperate

Ref gh-3732

* Tests: Reduce the abort timeout for simple focus testing

Ref gh-3732

* Tests: Simulate events when CI hinders use of native ones

Ref gh-3732

* Tests: Account for TestSwarm focus issues

Closes gh-3732

* Ajax: add an ontimeout handler to all requests

Fixes gh-3586
Close gh-3590

* Dimensions: Improve offsetWidth/offsetHeight fallback

Fixes gh-3698
Fixes gh-3602
Closes gh-3738

* Tests: Replace non-ASCII whitespace in source code

* Dimensions: Don't trust non-pixel computed width/height

Fixes gh-3611
Closes gh-3741

* Build: Fix comment typo

Closes gh-3747

* Build: Update my name in .mailmap

I got married! 🎉

* Build: Update my name in ATHORS.txt

I forgot .mailmap isn't everything.

* Tests: Update path calculation

Fixes gh-3756
Closes gh-3757

* CSS: Avoid unit-conversion interference from CSS upper bounds

Fixes gh-2144
Closes gh-3745

* Tests: Update lineHeight adjustments to give Android more slop

* CSS: Detect more WebKit styles erroneously reported as percentages

Ref 692f9d4
Fixes gh-3777
Closes gh-3778

* Build: Update to Babel 7, use for-of plugin instead of preset-es2015

Closes gh-3786

* Build: Drop cross-spawn, use child_process.spawn shell option

* Build: increase timeout in Promises/A+ tests 10 times

The promises-aplus-tests sets up a default 200 ms Mocha timeout. This makes
our tests randomly fail on Jenkins. 2 seconds will be safer.

Closes gh-3791

* Build: Remove package-lock.json, add it to .gitignore

npm 5, even the version included in the latest Node.js 8.5.0 re-generates
`package-lock.json` on each install. And when it does on a system that doesn't
support all the optional dependencies that are supported on the OS where the
lockfile was generated, it removes those optional deps from the lockfile.

The effect is that everyone firing `npm install` on our repo on any OS other
than macOS will immediately get a dirty state of the repo as the `fsevents`
dependency subtree gets removed from `package-lock.json`. That's a really bad
experience.

This commit removes package-lock.json from the repository and adds it to
.gitignore. We'll start committing the file again once the issue is resolved
on npm's part.

Fixes gh-3792

* Tests: Make Node tests work for paths with spaces in them

Without this patch Jenkins tests fail as jQuery job names there contain spaces,
e.g. "jQuery Core".

Closes gh-3821

* Tests: Add Safari 11 support test results

* Build: Test on Node.js 9

Closes gh-3840

* Tests: Add iOS 11 support test results

* Manipulation: Reduce size by eliminating single-use variable

Closes gh-3851

* CSS: Correctly set support properties with non-default zoom

Fixes gh-3808
Closes gh-3872

* Docs: Create CODE_OF_CONDUCT.md

Close gh-3865

* Tests: Add support for running unit tests via grunt with karma

- Update QUnit to 1.23.1
- Remove unused dl#dl from test/index.html
- Remove unused map#imgmap from test/index.html
- Ensure all urls to data use baseURI
- Add the 'grunt karma:main' task
  - customContextFile & customDebugFile
- Add 'npm run jenkins' script

Close gh-3744
Fixes gh-1999

* Build: Only run browser tests in one Node version on Travis

Ref gh-3744
Closes gh-3894

* Core: make camelCase function available only for internal usage

Close gh-3604
Fixes gh-3384

* Core: adjust data tests to ensure proper camelCasing

- Add back camelCase to the public object (deprecate not remove)
Ref #3384

* Core: deprecate jQuery.now

Fixes gh-2959
Close gh-3884

* Core: deprecate jQuery.proxy (not slated for removal)

Fixes gh-2958
Close gh-3885

* Manipulation: use `.children` to select tbody elements

- selectors beginning with a child combinator are not valid natively.
  This fixes the tests when using selector-native.js

* Attributes: allow array param in add/remove/toggleClass

+30 bytes instead of +182

Thanks to @faisaliyk for the first pass on this feature.

Fixes gh-3532
Close gh-3917

* Ajax: add unit test for getScript(Object)

Fixes gh-3736
Close gh-3918

* Tests: only run ontimeout test if ontimeout exists

Fixes gh-3742
Close gh-3919

* Build: Fix UglifyJS output in Android 4.0; update uglify

- Thanks to @mgol for first pass

Fixes gh-3743
Close gh-3920

* Tests: fix function reference for unbinding

Ref gh-2958

* Build: Remove CRLF line endings to fix builds on Windows

Close gh-3929

* Core: deprecate jQuery.isFunction

Fixes gh-3609

* Event: Move event aliases to deprecated

Fixes gh-3214

* Ajax: Don't process non-string data property on no-entity-body requests

Fixes gh-3438
Closes gh-3781

* Core: deprecate jQuery.isNumeric

Fixes gh-2960
Closes gh-3888

* Tests: fix weird failure in Edge 16 CSS

Fixes gh-3866
Close gh-3932

* Tests: fix weird flaky attributes test in Edge 16

Fixes gh-3867
Close gh-3931

* Core: deprecate jQuery.type

Fixes gh-3605
Close gh-3895

* Tests: fix number of expected assertions in basic core

* Tests: temporarily require sudo access for karma:main on travis

- This should fix the broken travis build on Node 8
- See travis-ci/travis-ci#8836

* Tests: correctly set sudo in travis config, not karma config

* Manipulation: Add support for scripts with module type

Fixes gh-3871
Close gh-3869

* Tests: fix tests in AMD mode

* Tests: ensure that module assertions run on supported browsers

- Also fixes tests for karma, where the URL for the module is different

Ref gh-3871

* Filter: Use direct filter in winnow

for both simple and complex selectors

Fixes gh-3272
Closes gh-3910

* Build: Add "-debug" suffix to name of karma debug tasks

Ref gh-3922
Close gh-3936

* Tests: skip test with invalid selector for selector-native tests

* Release: add new authors to AUTHORS.txt

* Release: update version to 3.3.0-pre

* Build: Updating the master version to 3.3.1-pre.

* Build: Updating the master version to 3.3.2-pre.
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
24ec4cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment