Closed
Bug 1160635
Opened 11 years ago
Closed 11 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•11 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•11 years ago
|
Flags: needinfo?(dbaron)
|
Assignee | |
Comment 2•11 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•11 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•11 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•11 years ago
|
Assignee: nobody → dbaron
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
|
|
Assignee | |
Comment 5•11 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•11 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•11 years ago
|
||
The reftests pass with the patch, based on local testing.
Attachment #8601406 -
Flags: review?(dholbert)
|
|
Assignee | |
Comment 8•11 years ago
|
||
|
||
Comment 9•11 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•11 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•11 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•11 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•11 years ago
|
||
|
||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54a0e4f9c82f
https://hg.mozilla.org/mozilla-central/rev/78435ee2a9a5
Status: NEW → RESOLVED
Closed: 11 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
•