Closed
Bug 1131812
Opened 10 years ago
Closed 10 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)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: dbaron, Assigned: dbaron)
Details
Attachments
(3 files)
569 bytes,
text/html
|
Details | |
25.12 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
4.33 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•10 years ago
|
||
... or maybe not since it's 47px rather than 20px.
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 3•10 years ago
|
||
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.)
Assignee | ||
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8564837 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5e670610fec2
https://hg.mozilla.org/mozilla-central/rev/c38eaa958c1e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 13•10 years ago
|
||
Tests migrated to CSSWG repository in https://hg.csswg.org/test/rev/eeefc17e6d6c .
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•