Last Comment Bug 585069 - xul images should support ratio scaling
: xul images should support ratio scaling
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla8
Assigned To: Neil Deakin
:
Mentors:
Depends on: 677087 677091 677161 677886
Blocks: Instantbird
  Show dependency treegraph
 
Reported: 2010-08-06 08:37 PDT by Neil Deakin
Modified: 2011-12-30 12:49 PST (History)
18 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
patch, partially tested, automated tests to come (11.13 KB, patch)
2010-08-06 08:37 PDT, Neil Deakin
no flags Details | Diff | Review
patch with reftests (16.57 KB, patch)
2011-03-07 10:50 PST, Neil Deakin
dbaron: review-
Details | Diff | Review
patch with proper border/padding handling (23.75 KB, patch)
2011-06-03 12:37 PDT, Neil Deakin
dbaron: review+
Details | Diff | Review

Description Neil Deakin 2010-08-06 08:37:51 PDT
Created attachment 463557 [details] [diff] [review]
patch, partially tested, automated tests to come

Such that:

<image src="image.png" maxwidth="400" maxheight="300"/>

can specify maximum sizes and the image will shrink down with the same ratio in both directions.
Comment 1 Neil Deakin 2011-03-07 10:50:01 PST
Created attachment 517458 [details] [diff] [review]
patch with reftests
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-21 22:20:00 PDT
Comment on attachment 517458 [details] [diff] [review]
patch with reftests

Sorry for taking so long to get to this (though for a while I was
postponing it because your bugzilla name said you were on vacation).

'width' and 'height' should not be inputs to
nsLayoutUtils::ComputeAutoSizeWithIntrinsicDimensions; they should
just be local variables inside of it.  (Or, alternatively, they
could be out params and the function could return void, but it's
probably better returning nsSize as you have it.)

In nsLayoutUtils.h, ComputeAutoSizeWithIntrinsicDimensions should
give a better explanation of tentWidth and tentHeight:  it should
describe them as the result of applying the rules for handling
intrinsic sizes and ratios.

The way you use this in nsImageBoxFrame seems to me to all be off by
border+padding.  In particular, the code in nsLayoutUtils that
you're using intends to deal with content-box sizes, but you're
giving it border-box sizes for the min and max sizes (and for the
ignored first two parameters), though you're giving it content box
sizes for the important last 2 parameters.  I *think* that's going
to yield the wrong results.  Then the code in nsLayoutUtils returns
a content box size, but you're returning it directly from
GetPrefSize which is supposed to return a border-box size.

The hand-written code in nsImageBoxFrame seems likewise incorrect in
the presence of border and/or padding.

I didn't look closely at your tests, but I think you need a bunch
with border and padding; you don't seem to have any.
Comment 3 Neil Deakin 2011-06-03 12:37:36 PDT
Created attachment 537208 [details] [diff] [review]
patch with proper border/padding handling

This patch subtracts off and adds on the border/padding as needed.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-04 13:53:56 PDT
Comment on attachment 537208 [details] [diff] [review]
patch with proper border/padding handling

I recommend also having tests with asymmetric padding and border, e.g.,
with padding: 1px 2px 4px 8px.

+    if (size.width != NS_INTRINSICSIZE)
+      size.width += borderPadding.LeftRight();
+    if (size.height != NS_INTRINSICSIZE)
+      size.height += borderPadding.TopBottom();

It shouldn't be possible for these to be NS_INTRINSICSIZE, unless
I'm missing something (which I think would also mean things are
pretty broken).

+      nscoord height = size.height;
+      if (height != NS_INTRINSICSIZE)
+        height -= borderPadding.TopBottom();

Same here.

+      nscoord width = size.width;
+      if (width != NS_INTRINSICSIZE)
+        width -= borderPadding.LeftRight();

And here.

Instead of these tests, could you assert that |size| doesn't have
NS_INTRINSICSIZE components both after AddCSSPrefSize and after
ComputeAutoSizeWithIntrinsicDimensions?

r=dbaron with that

Sorry for the delay getting to this.
Comment 5 Marco Bonardo [::mak] 2011-08-06 02:56:02 PDT
http://hg.mozilla.org/mozilla-central/rev/7fece42cb99a
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-16 14:20:30 PDT
(tracking-8+ since we might need to back this out on aurora if bug 677091 and related issues don't get fixed)
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-19 15:10:19 PDT
This patch added tests to the reftest directory layout/xul/base/reftest/ (originally added in bug 442228) which was not listed in the toplevel manifest, so they were never run in our automated testing.  I just did that:
https://hg.mozilla.org/integration/mozilla-inbound/rev/139eed687d77
Comment 8 Eric Shepherd [:sheppy] 2011-08-24 08:28:27 PDT
Do we know yet whether this will make Firefox 7? I see there's some concern about possibly backing it out.
Comment 9 Neil Deakin 2011-08-24 09:15:27 PDT
The regression problems were fixed so it should be staying in.
Comment 10 Simona B [:simonab] 2011-10-05 07:59:56 PDT
Can anyone tell me how can this be verified?
Comment 11 Neil Deakin 2011-10-05 08:14:45 PDT
This bug changes the following behaviour. Given an example such as:

<image src="image.png" maxwidth="200"/>

where image.png is a large image such as a photo, the old behaviour was to shrink the width down to the maximum allowed of 200 but keep the height the same. The new behaviour aligns more with the html img tag and shrinks both the width and height down proportionally.

An automated test also verifies the behaviour.
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 09:35:54 PDT
Given this is covered by automation, I don't think there is much QA can do to verify this is fixed. Simona, if you feel like you have time, you can code an example like in comment 11 (test it in a previous build and the current build to compare).

Note You need to log in before you can comment on or make changes to this bug.