Closed Bug 1131812 Opened 5 years ago Closed 5 years ago

SVG as image with an intrinsic width and max-height has its width incorrectly scaled down by the same ratio as the height is scaled down

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(3 files)

In the second test item in the box-sizing-replaced-element-001.html in
https://lists.w3.org/Archives/Public/www-style/2015Feb/0243.html
we have a bug.

It looks like we're incorrectly applying the box-sizing property to a width attribute that is *inside* an SVG-as-image.

(If it were an embedded SVG, it should apply.)

(Sorry, filing quickly during a CSS+SVG WG meeting.)
... or maybe not since it's 47px rather than 20px.
This isn't actually related to box-sizing.
No longer blocks: 593964
Summary: box-sizing incorrectly applies to a width inside an svg-as-image → odd sizing of an svg-as-image
Summary: odd sizing of an svg-as-image → SVG as image with an intrinsic width and max-height is scaled down by its ratio when only the height should be scaled down
Summary: SVG as image with an intrinsic width and max-height is scaled down by its ratio when only the height should be scaled down → SVG as image with an intrinsic width and max-height has its width incorrectly scaled down by the same ratio as the height is scaled down
I think the basic problem is that we're using some or all of nsLayoutUtils::ComputeAutoSizeWithIntrinsicDimensions without first checking that the element has an intrinsic ratio.

The big table in the spec at
http://dev.w3.org/csswg/css2/visudet.html#min-max-widths
is conditioned on "However, for replaced elements with an intrinsic ratio and both 'width' and 'height' specified as 'auto', the algorithm is as follows:", and I think we're using it without checking the intrinsic ratio in the call from nsLayoutUtils::ComputeSizeWithIntrinsicDimensions.  (Should look at other callers too.)
Attached file testcase 1
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
This is because the rules in the table in section 10.4 of CSS 2.1 only
apply when width and height are auto (which we already tested) and when
the image has an intrinsic ratio (which we did not).

I had hoped to simplify the code further, but there are tricky ordering
dependencies that make it hard to do.  (In particular, in the branches
for one dimension auto and one non-auto, the non-auto dimension is
computed first.)
Attachment #8564838 - Flags: review?(dholbert)
Comment on attachment 8564837 [details] [diff] [review]
patch 1 - Add reftests for sizing of replaced elements

>+++ b/layout/reftests/w3c-css/submitted/css21/replaced-sizing/replaced-elements-all-auto-ref.html
>@@ -0,0 +1,36 @@
>+<!DOCTYPE html>
>+<html lang="en-US">
>+<head>
>+  <title>CSS Test: CSS 2.1 replaced element sizing</title>
[...]
>+++ b/layout/reftests/w3c-css/submitted/css21/replaced-sizing/replaced-elements-all-auto.html
>@@ -0,0 +1,30 @@
>+<!DOCTYPE html>
>+<html lang="en-US">
>+<head>
>+  <title>CSS Test: CSS 2.1 replaced element sizing</title>

Nit: looks like this testcase & reference case have the same <title>. (& same goes for the others)  Should the reference cases perhaps have a different title? (This makes it easier to distinguish them when switching between them in tabs -- you can just look at the browser titlebar.) The minimal templates at http://testthewebforward.org/docs/test-templates.html#including-styles do seem to imply that references should have a different title, with "${1:Test title}" vs. "${1:Reference title}".

So: maybe the reference case files just want s/Test/Reference/ in their <title> element?

r=me w/ this tweaked or not, as you see fit.
Attachment #8564837 - Flags: review?(dholbert) → review+
Comment on attachment 8564838 [details] [diff] [review]
patch 2 - Don't do ratio scaling when applying min/max-width/height for images without an intrinsic ratio

>+++ b/layout/base/nsLayoutUtils.cpp
>+      if (aIntrinsicRatio != nsSize(0, 0)) {
>+        nsSize autoSize =
>+          ComputeAutoSizeWithIntrinsicDimensions(minISize, minBSize,
>+                                                 maxISize, maxBSize,
>+                                                 tentISize, tentBSize);
>+        // The nsSize that ComputeAutoSizeWithIntrinsicDimensions returns will
>+        // actually contain logical values if the parameters passed to it were
>+        // logical coordinates, so we do NOT perform a physical-to-logical
>+        // conversion here, but just assign the fields directly to our result.
>+        iSize = autoSize.width;
>+        bSize = autoSize.height;
>+      } else {
>+        iSize = NS_CSS_MINMAX(tentISize, minISize, maxISize);
>+        bSize = NS_CSS_MINMAX(tentBSize, minBSize, maxBSize);
>+      }

So it seems the main issue here was that ComputeAutoSizeWithIntrinsicDimensions *assumes* that we have an intrinsic ratio, right? (i.e. it assumes that if we clamp tentBSize, we need to do similar clamping on tentISize -- to the extent allowed by its own min/max requirements)

Might be worth a comment somewhere making that ComputeAutoSizeWithIntrinsicDimensions requirement / assumption more explicit.  Maybe it'd be clearer w/ just a comment in the "else" case here, explaining why we're not bohtering with ComputeAutoSizeWithIntrinsicDimensions, like:

  // No intrinsic ratio --> we can just clamp each dimension's tentative size
  // independently, w/o worrying about having to propagate to other dimension.

r=me regardless
Attachment #8564838 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/5e670610fec2
https://hg.mozilla.org/mozilla-central/rev/c38eaa958c1e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.