Closed
Bug 1160635
Opened 9 years ago
Closed 9 years ago
height/width CSS values not recalculating when changing <img> src
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jacobhaigh, Assigned: dbaron)
References
Details
(Whiteboard: [parity-chrome][parity-IE][parity-presto opera])
Attachments
(3 files)
994 bytes,
text/html
|
Details | |
4.41 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.69 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36 Steps to reproduce: I can reproduce by following these steps: create an html document 2 divs with 1 img in each. div style: width=100px. img 1 style: width=100% img 2 style: width=100% height=100% now use javascript to change src for each image. http://jsfiddle.net/5r0yet2x/12/ Actual results: the height of img1 stays the calculated height for the first src. img2 has height recalculated for the new src. http://jsfiddle.net/5r0yet2x/12/ Expected results: both should have their height readjusted. chrome has this outcome. http://jsfiddle.net/5r0yet2x/12/
Comment 1•9 years ago
|
||
The behavior has been changed since landing Bug 300030
Blocks: reflow-refactor
Component: Untriaged → Layout
Product: Firefox → Core
Whiteboard: [parity-chrome][parity-IE][parity-presto opera]
Updated•9 years ago
|
Flags: needinfo?(dbaron)
Assignee | ||
Comment 2•9 years ago
|
||
Steps to reproduce: 1. click on the first image Expected results: 1. image increases in height Actual results: 1. image does not increase in height (It would have been nice to say explicitly that the steps to reproduce involve clicking on the image rather than the refresh button. I also strongly prefer having testcases attached so that we don't have to worry about them changing or disappearing.)
Assignee | ||
Comment 3•9 years ago
|
||
My guess is that the function HaveFixedSize(const nsHTMLReflowState&) in nsImageFrame.cpp is incorrectly returning true, causing the IMAGE_SIZECONSTRAINED flag to be set in nsImageFrame::Reflow.
Assignee | ||
Comment 4•9 years ago
|
||
I think the HaveFixedSize's assumptions were broken by the reflow branch. That said, I think the assumptions we're making around IMAGE_SIZECONSTRAINED are probably also broken. For example, if the element has a percentage width, but its parent has width: -moz-max-content, the assumptions are wrong; we'd need to report a change in intrinsic widths. I think we should simply stop making that optimization for percentage widths and heights since there are too many complicated cases to worry about.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
Assignee | ||
Comment 5•9 years ago
|
||
I have a patch to fix this, with reftests for both this height case and an intrinsic width case that was broken. My tree was out-of-date, so I need to wait for it to build before testing and uploading the tests and patch.
Assignee | ||
Comment 6•9 years ago
|
||
I confirmed that without patch 2, the reftests fail in the expected way (with the image in the test having 100px height/width instead of 50px).
Attachment #8601405 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•9 years ago
|
||
The reftests pass with the patch, based on local testing.
Attachment #8601406 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d21eadfaf558
Comment 9•9 years ago
|
||
Comment on attachment 8601405 [details] [diff] [review] patch 1 - Add reftests >+++ b/layout/reftests/image/image-resize-percent-height.html >@@ -0,0 +1,14 @@ >+<!DOCTYPE HTML> >+<html class="reftest-wait"> >+<script> >+function run() { >+ var img = document.getElementsByTagName("img")[0]; >+ img.onload = imgload; >+ img.src = "blue-50x50.png"; >+} >+function imgload() { >+ document.documentElement.classList.remove("reftest-wait"); >+} >+</script> >+<body onload="run()"> Nit: Perhaps the tests should use MozReftestInvalidate instead of <body onload>, for definitely-testing-the-bug robustness? (I thought it was possible [albeit rare] that we might fire onload before completing reflow. And if that were to happen here, it'd let us avoid exercising the buggy codepath. Maybe I'm misremembering though.) r=me regardless
Attachment #8601405 -
Flags: review?(dholbert) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8601406 [details] [diff] [review] patch 2 - Stop making image resizing optimzation when image has percent width or height ># HG changeset patch ># User L. David Baron <dbaron@dbaron.org> ># Date 1430819190 -7200 ># Tue May 05 11:46:30 2015 +0200 ># Node ID b5dafe03259e864113ee7df5d42d318302ca0811 ># Parent ca2320c18f4b1615629064bf61ece0092b816cce >Bug 1160635 patch 2 - Stop making image resizing optimzation when image has percent width or height. Typo: s/optimzation/optimization/ >+++ b/layout/generic/nsImageFrame.cpp >+// Decide whether we can optimize away reflows that result from the >+// image's intrinsic size changing. > inline bool HaveFixedSize(const nsHTMLReflowState& aReflowState) > { [...] >+ return height.IsCoordPercentCalcUnit() && >+ !height.HasPercent() && It looks like 'height.ConvertsToLength()' is a more concise (and slightly more efficient) way of expressing this same check, I think. It's implemented like so: 146 bool ConvertsToLength() const { 147 return mUnit == eStyleUnit_Coord || 148 (IsCalcUnit() && !CalcHasPercent()); 149 } http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleCoord.h?rev=ac4464790ec4#146 So, probably best to condense the height/width checks here to use that, unless I'm missing something. >@@ -485,17 +479,17 @@ nsImageFrame::ShouldCreateImageFrameFor( > useSizedBox = true; > } > else if (aStyleContext->PresContext()->CompatibilityMode() != > eCompatibility_NavQuirks) { > useSizedBox = false; > } > else { > // check whether we have fixed size >- useSizedBox = HaveFixedSize(aStyleContext->StylePosition()); >+ useSizedBox = HaveSpecifiedSize(aStyleContext->StylePosition()); > } s/fixed/specified/ in the comment here, to avoid confusion, since we still have a "HaveFixedSize" function. r=me w/ the above
Attachment #8601406 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #9) > Nit: Perhaps the tests should use MozReftestInvalidate instead of <body > onload>, for definitely-testing-the-bug robustness? > > (I thought it was possible [albeit rare] that we might fire onload before > completing reflow. And if that were to happen here, it'd let us avoid > exercising the buggy codepath. Maybe I'm misremembering though.) I think I prefer doing img.offsetWidth to ensure we've done reflow. I'll do that. (In reply to Daniel Holbert [:dholbert] from comment #10) > r=me w/ the above Yep, I'll do all of those.
Comment 12•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #11) > I think I prefer doing img.offsetWidth to ensure we've done reflow. I'll do > that. Sounds good, thanks!
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a0e4f9c82f https://hg.mozilla.org/integration/mozilla-inbound/rev/78435ee2a9a5
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54a0e4f9c82f https://hg.mozilla.org/mozilla-central/rev/78435ee2a9a5
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•