Closed Bug 831780 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

18 Branch
x86_64
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox18 --- affected
firefox19 --- verified
firefox20 --- verified
firefox21 --- verified
b2g18 --- affected
b2g18-v1.0.1 --- affected

People

(Reporter: dtrebbien, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file 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
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
Correction:  Mac OS 10.7.5 and Windows *7* tested
Attachment #703332 - Attachment mime type: text/plain → text/html
Component: DOM: Core & HTML → Layout
Presumably the overflow area is different for some reason...
That said, a current nightly build seems to show correct behavior.
Er, nevermind.  I see it now.
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
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: dlbi
Attached file Simple testcase
Flags: needinfo?(roc)
Flags: needinfo?(matt.woodrow)
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
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)
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
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [need review]
Target Milestone: --- → mozilla21
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
QA Contact: simona.marcu
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also 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
Attachment #703734 - Flags: approval-mozilla-release?
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: RESOLVED → VERIFIED
Depends on: 939564
You need to log in before you can comment on or make changes to this bug.