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)

Product:
Core
Component:
Layout
Version:
37 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: jacobhaigh, Assigned: dbaron)

References

Details

(Whiteboard: [parity-chrome][parity-IE][parity-presto opera])

Attachments

(3 files)

reporter's testcase
994 bytes, text/html
Details
patch 1 - Add reftests
4.41 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
patch 2 - Stop making image resizing optimzation when image has percent width or height
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/
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]
Flags: needinfo?(dbaron)
Attached file reporter's testcase — 
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.)
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.
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: nobody → dbaron
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
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.
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)
The reftests pass with the patch, based on local testing.
Attachment #8601406 - Flags: review?(dholbert)
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 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+
(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.
(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!
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.

Attachment

General

Creator:
Created:
Updated:
Size: