Per bug 624647 comment 34, one utility function that we use for "object-position" -- ComputeObjectAnchorPoint -- gives us an anchor point, along with a target rect to render into. My patches in bug 624647 don't respect the anchor point yet. Filing this bug here on respecting the anchor point. STR: 0. Build with bug 624647's patches. (not yet landed, as of this bug being filed) 1. Load attached testcase & reference case. 2. Compare their renderings. EXPECTED RESULTS: Renderings should match exactly. ACTUAL RESULTS: Renderings *do not quite match*. Specifically: - In the testcase, the image is biased to show a bit more of the bottom half (more salmon/lime). - In the reference case, the image is biased to show a bit more of the top half (more blue/black). This is just a rounding difference, due to honoring the anchor point for background-position in the reference case, but not for object-position (yet) in the testcase. (I verified that this is what's responsible for the difference by neutering the anchor-point in ComputeObjectAnchorPoint & making it just return the target rect's upper-left corner. That makes the reference case change its rendering to match the testcase.)

I have a WIP patch that fixes testcase 1, but no this second testcase (whose reference is coming up).

This patch: (A) Makes nsLayoutUtils::ComputeObjectDestRect() (the "object-fit/position" computation function) return the anchor point via an (optional) outparam. (Might end up making it required -- not optional -- but for now, it makes the patch-stack cleaner if we don't force all the callers to update right away.) (B) Makes nsLayoutUtils::DrawSingleImage() accept an anchor-point input parameter. By default, it uses the top-left corner of the image; but the call-site in nsImageFrame::PaintImage() needs more flexibility than that now. (We'll make that callsite actually pass an anchor-point in the next patch.) (A and B are sort of logically separate, but they're both small & in the same file, so I combined them into a single patch.)

This patch captures the ComputeObjectDestRect() anchor-point, and passes it off to DrawSingleImage, in nsImageFrame's painting code. (We need something like this for the other container-frames that call ComputeObjectDestRect, too; just posting this one for now. The others will be a bit trickier.)

(Both attached testcases pass their references, with attached patch. Official reftests patch still to come.)

Comment on attachment 8525125 [details] [diff] [review] part 1 v1: Add optional anchor-point outparam to nsLayoutUtils::ComputeObjectDestRect() and in-param to nsLayoutUtils::DrawSingleImage() Review of attachment 8525125 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: layout/base/nsLayoutUtils.h @@ +1645,5 @@ > const nsRect& aDirty, > const mozilla::SVGImageContext* aSVGContext, > uint32_t aImageFlags, > + const nsRect* aSourceArea = nullptr, > + const nsPoint* aAnchorPoint = nullptr); IMO you should switch the order here. I'd expect anchor points to be more widely used than aSourceArea. (Granted that does mean you have to update the one caller that uses aSourceArea.)

Comment on attachment 8525128 [details] [diff] [review] part 2: Bug 1098417 part 2: Make nsImageFrame pass "object-position"-determined anchor-point to DrawSingleImage Review of attachment 8525128 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: layout/generic/nsImageFrame.cpp @@ +1549,5 @@ > > nsLayoutUtils::DrawSingleImage(*aRenderingContext.ThebesContext(), > PresContext(), aImage, > nsLayoutUtils::GetGraphicsFilterForFrame(this), dest, aDirtyRect, > + nullptr, aFlags, nullptr, &anchorPoint); See what I mean about how nobody uses aSourceArea? =)

Thanks for the review! Unfortunately, the Try run shows some test-failures, which in retrospect I should have anticipated. I'm not sure we actually can make this change (respecting anchor points) after all. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=93fded244cc1 Reasoning on why this is causing failures: - Traditionally, for an unstyled <img>, we've used its upper-left corner as its pixel-aligned "anchor". This seems like intuitive behavior & is worth preserving (for an unstyled <img>). - BUT: now that we have "object-position" support, an unstyled <img> will have the default value "object-position: 50% 50%", which gives us an anchor-point at the *center* (should we choose to respect it). - So in cases where pixel-alignment isn't trivial, this will *change our rendering* of an unstyled <img>, potentially making the top and/or bottom pixels fuzzy/smeared (as is the case for e.g. "image-size.xul" in the reftest run -- the reference case is actually what's getting fuzzier there) For cases where someone specifies e.g. "object-position: 100% 100%", it seems reasonable that we should use the bottom-left corner as the anchor point. But for the *default value* of "object-position: 50% 50%", it seems like we have to use the upper-left corner as our anchor (not the center), to avoid breaking backwards-compatibility on <img> elements that are completely unaware of "object-position". Not sure if this means we should ignore the anchor-point altogether, or just make an exception for "50% 50%". Probably the latter.

Here's the updated "part 1", with comment 7 addressed (swapping the optional-arg order).

Here's a "part 1.5" patch, which makes us *ignore* the computed anchor-point and instead use the upper-left corner, if object-fit & object-position are both at their initial values. This is a bit of a hack, but it's necessary to address the backwards-compat issues (and reftest failures) mentioned in comment 9.

(updated version of part 2, w/ arg-ordering swapped per comment 7 / comment 8.)

Here's a patch with reftests for this bug, with template files & a "generate" script, as in bug 624647. This patch generates tests using two PNG image files (one wide, one skinny). (Subsequent reftest patches will add similar tests, with SVG images & WebM videos. I won't request review on those ones, since they'll basically be copies of these ones, just with different media.)

As in bug 624647, here's a minimized version of the reftests patch, with just one actual sample test-file. Requesting review. Note: The "generate" script is generated via "hg cp" from bug 624647's "generate" script, so only the diffs are shown in the patch. In particular: - I dropped the outer loop (over 'object-fit' values), since we don't need to bother testing different object-fit values to exercise anchor-point-honoring. - As explained in a large comment in the script, each generated test covers *either* object-position x-values ("op_x") *or* object-position y-values ("op_y"). The script deletes the templates' classes/elements for the un-tested dimension (via sed & "$classPatternToDelete").

This patch uses an identical script as in part 3 (and hence generates identical test/reference files), except it uses SVG images instead of PNG images. Hence, not requesting a separate review on this. (As annotated in this patch's reftest.list chunk, some of these tests don't pass yet -- specifically, the container-elements <object> & <embed>, which don't use nsImageFrame when given SVG content. That's covered in followup bug 1103286.)

(And here's a patch that generates <video> tests with WebM content, via 'hg cp' from part 3's tests for <video poster>. The generate script is just a tweaked version of the similar script from bug 624647. These tests don't pass yet, since nsVideoFrame doesn't honor the anchor point yet; I filed bug 1104298 for that & mentioned it in reftest.list here.)

Try run w/ updated patch-stack: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=533d55027df8

Comment on attachment 8527896 [details] [diff] [review] part 1.5: Make ComputeObjectDestRect ignore computed anchor point, for initial property-values Review of attachment 8527896 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! This is definitely the way to go; we should prefer having the anchor point in the top-left corner whenever possible. To do otherwise will probably lead to rendering regressions. ::: layout/base/nsLayoutUtils.cpp @@ +3624,5 @@ > return aConstraintSize; // fall back to (default) 'fill' behavior > } > } > > +// (Helper for HasInitialObjectFitAndPosition, to check each "object-posiiton" coord.) posiiton -> position @@ +3675,5 @@ > if (aAnchorPoint) { > + // Special-case: if our "object-fit" and "object-position" properties have > + // their default values ("object-fit: fill; object-position:50% 50%"), then > + // we'll override the calculated imageAnchorPt, and instead use the object's > + // top-left corner, for backwards-compatibility. Maybe specify that it's not just about backwards compatibility, but also because nsLayoutUtils::ComputeSnappedDrawingParameters produces less error for the special case of having the anchor point in the top left corner. As you mentioned on IRC, in theory with object-fit: fill any percentage values are the same, but in practice we have a different code path for the top left corner anchor point case.

Comment on attachment 8527959 [details] [diff] [review] part 3: reftests, *minimized* for review Review of attachment 8527959 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

Thanks! Tweaked comments per comment 18, & landed: part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc9f527fa12 part 1.5: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa382f6ee7ba part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1bc74871c0 part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ac2902afded part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec328e9feda part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f28d2815117

https://hg.mozilla.org/mozilla-central/rev/8bc9f527fa12 https://hg.mozilla.org/mozilla-central/rev/aa382f6ee7ba https://hg.mozilla.org/mozilla-central/rev/ea1bc74871c0 https://hg.mozilla.org/mozilla-central/rev/7ac2902afded https://hg.mozilla.org/mozilla-central/rev/3ec328e9feda https://hg.mozilla.org/mozilla-central/rev/5f28d2815117