scrollWidth, scrollHeight of DIV containing IMG reported as 40x40px until the image's actual size is known, regardless of CSS sizing of the IMG

VERIFIED FIXED in Firefox 19

Status

()

--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: dtrebbien, Assigned: bzbarsky)

Tracking

({regression})

18 Branch
mozilla21
x86_64
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 affected, firefox19 verified, firefox20 verified, firefox21 verified, b2g18 affected, b2g18-v1.0.1 affected)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 703332 [details]
Test case

When an IMG element is first added to a DIV and the referenced image is not cached, the scrollWidth, scrollHeight of the parent DIV is calculated as 40x40px until the image's actual size is known, even if width/height attributes are specified on the IMG or the IMG is explicitly sized via CSS.

This is an issue first affecting Firefox 18.0.  Firefox 17.0.1 and Firefox ESR 17.0.2 are unaffected.

The presence of a DOCTYPE does not appear to matter.

The issue affects data URIs as well as images that are not in the browser cache.

Attached is a reproducible test case using various sizes of an image from Wikimedia Commons.  If you open the test case in Firefox 18.0 (Mac OS 10.7.5 and Windows 8 tested), the following is printed to the Web Console
Actual:
0: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16

Expected:
0: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16


I have also included code for downloading the images from Wikimedia Commons.  If you comment out the line setting the testImg src to a data URI and uncomment one of the lines setting the src to a Wikimedia Commons URL, you will see "0: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40" printed to the console until the image is cached.  Holding Shift while clicking the refresh button causes "0: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40" to always be printed.

Experimenting with Fiddler 2 to rate limit the download of the full-resolution image, I see that for the first several seconds, the DIV's scrollWidth and scrollHeight are 40px, but then the scrollWidth and scrollHeight are corrected:

[08:18:50.617] 0: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:51.612] 1: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:52.623] 2: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:53.615] 3: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:54.616] 4: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:55.617] 5: testDiv.scrollWidth = 40, testDiv.scrollHeight = 40,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:56.619] 6: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:18:57.621] 7: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
...
[08:20:08.623] 78: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
[08:20:09.617] 79: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
(Reporter)

Comment 1

6 years ago
Results for other browsers:

Safari 6.0.2:
0: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16

IE 8:
LOG: 0: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
LOG: 1: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
LOG: 2: testDiv.scrollWidth = 16, testDiv.scrollHeight = 16,
testImg.offsetWidth = 16, testImg.offsetHeight = 16

Opera 12.12:
0: testDiv.scrollWidth = 16, testDiv.scrollHeight = 17,
testImg.offsetWidth = 16, testImg.offsetHeight = 16
(Reporter)

Comment 2

6 years ago
Correction:  Mac OS 10.7.5 and Windows *7* tested
(Assignee)

Updated

6 years ago
Attachment #703332 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

6 years ago
Component: DOM: Core & HTML → Layout
Keywords: regressionwindow-wanted
(Assignee)

Comment 3

6 years ago
Presumably the overflow area is different for some reason...
(Assignee)

Comment 4

6 years ago
That said, a current nightly build seems to show correct behavior.
(Assignee)

Comment 5

6 years ago
Er, nevermind.  I see it now.
(Assignee)

Comment 6

6 years ago
This first appeared as a problem back in September, in this checkin range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=895f66c4eada&tochange=c09a0c022b2e

There was then a behavior change in November that makes us return 24x24 instead of 40x40 or 16x16.  That was in http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dd68409d7810&tochange=a761bfc192b5

The first range has bug 539356 as a plausible culprit.

Nothing in the second range jumps out at me, though it does have some scrollframe changes...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regressionwindow-wanted → regression
(Assignee)

Comment 7

6 years ago
Ah, here we go.

The change to 24x24 was due to bug 810876.

The initial change was in fact due to bug 539356.

The problem is that the broken-image placeholder can in fact be larger than the image.

Matt, roc, should we be including the altfeedback stuff in visual but not layout overflow, perhaps?
Blocks: 539356
(Assignee)

Comment 8

6 years ago
Created attachment 703354 [details]
Simple testcase
(Assignee)

Updated

6 years ago
Flags: needinfo?(roc)
Flags: needinfo?(matt.woodrow)

Comment 9

6 years ago
Just a note on why this is a serious bug: this happens even if you provide sizes directly on the <img> tag (via HTML attributes or CSS), so we get a bad size from the browser even if we're explicitly telling the browser how big the image should be.

This in turn breaks the ability to determine the size of any content that includes images (inclusive of data URIs), which is a fundamental need of JavaScript frameworks that provide auto-sizing features.

We're really hoping for an 18.0.1 fix on this one, as the workarounds needed to get around this problem are really bad.
Actually, it's not quite clear to me why we're including the altfeedback in overflow at all.  It doesn't seem to be painted outside the image bounds...
I think we want to only modify the visual overflow, yes.

It definitely was painting outside of the image bounds, and causing reftest failures.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> I think we want to only modify the visual overflow, yes.

I agree.
Flags: needinfo?(roc)
Assignee: nobody → matt.woodrow
Created attachment 703734 [details] [diff] [review]
Only count the broken-image placeholder in the visual overflow, not the scrollable overflow.
Attachment #703734 - Flags: review?(matt.woodrow)
Charles, I'm going to try to get this into 20 and 19, but the chance of us doing an 18.0.x for something that's not an actively exploited security vulnerability or a problem affecting lots and lots of users in extremely visible ways is pretty slim.  I appreciate that this isn't ideal from your point of view, but note that Firefox 19 will be shipping on February 19, while it's unlikely that something like this would get into an 18.0.x release before probably February 1 or so at this point.

For what it's worth, having bugs like this reported against our Aurora or even Beta builds, _before_ we release would make it very likely that we'd also fix them before release...  That's why we have the various testing channels, so people can test those builds.  I know it's not always possible to do that, but it really helps when it's done.

Alex, Lukas, if we do plan to do an 18.0.x for other reasons anyway, we might want to consider this for a ridealong.  This should be an _extremely_ safe change.
Assignee: matt.woodrow → bzbarsky
Flags: needinfo?(lsblakk)
Flags: needinfo?(akeybl)
Whiteboard: [need review]
Attachment #703734 - Flags: review?(matt.woodrow) → review+
Comment on attachment 703734 [details] [diff] [review]
Only count the broken-image placeholder in the visual overflow, not the scrollable overflow.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08762b3f956

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 539356
User impact if declined: JS that tries to measure image sizes by measuring
   scrollable areas of elements containing those images will get wrong answers. 
   Might break some libraries/sites.
Testing completed (on m-c, etc.): Passes attached test.
Risk to taking this patch (and alternatives if risky): Very low risk, I believe.
   The alternative is to just let this ride the trains.
String or UUID changes made by this patch: None.
Attachment #703734 - Flags: approval-mozilla-release?
Attachment #703734 - Flags: approval-mozilla-beta?
Attachment #703734 - Flags: approval-mozilla-aurora?
Comment on attachment 703734 [details] [diff] [review]
Only count the broken-image placeholder in the visual overflow, not the scrollable overflow.

Approving for branches and will leave the release  branch nom so this is on the radar in case of another 18 respin (though at present one doesn't not appear likely).
Attachment #703734 - Flags: approval-mozilla-beta?
Attachment #703734 - Flags: approval-mozilla-beta+
Attachment #703734 - Flags: approval-mozilla-aurora?
Attachment #703734 - Flags: approval-mozilla-aurora+
Flags: needinfo?(lsblakk)

Comment 17

6 years ago
Hi Boris, thanks for the comments re: release timing.  

At the moment I think this bug is probably causing a lot of issues on sites but few teams have figured out the underlying cause.  It's tricky to isolate since it goes away if images are cached.

I have asked our users to chime in if they know of prominent affected sites (not just ones based on our technology), which I think will help judge the urgency.
https://hg.mozilla.org/mozilla-central/rev/f08762b3f956
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [need review]
Target Milestone: --- → mozilla21
https://hg.mozilla.org/releases/mozilla-beta/rev/2a9c854f17c3

This is also in my queue to land on Aurora once it reopens.
status-b2g18: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
status-firefox20: --- → affected
status-firefox21: --- → fixed

Updated

6 years ago
Flags: needinfo?(akeybl)
Verified as fixed on Firefox 19 beta 3 - the scrollWidth and scrollHeight of the parent DIV are properly calculated when using the test cases attached in the Description and Comment 8.

Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.7.5:
Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130123083802
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130123083802
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130123083802
status-firefox19: fixed → verified
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
status-b2g18-v1.0.1: --- → affected
Verified as fixed on Firefox 20 beta 1 - scrollWidth and scrollHeight of the parent DIV are properly calculated

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:
Build ID: 20130220104816
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
status-firefox20: fixed → verified
Attachment #703734 - Flags: approval-mozilla-release?

Comment 24

6 years ago
Verified as fixed on Windows 7, Ubuntu 12.04 and Mac OSX 10.8.3, all 64bit, on Firefox 21 beta 6 (20130430204233).

scrollWidth and scrollHeight are properly computed when using the test case and simple test case attached to this bug.
status-firefox21: fixed → verified
Status: RESOLVED → VERIFIED
Depends on: 939564
You need to log in before you can comment on or make changes to this bug.