Closed Bug 1149357 Opened 9 years ago Closed 6 years ago

Not setting a width and height on srcset image causes incorrect width for shrink-wrapping ancestor

Categories

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

Product:
Core
Component:
Layout: Images, Video, and HTML Frames
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- verified

People

(Reporter: marcosc, Assigned: emilio)

References

(Blocks 3 open bugs,
URL
)

Details

(Whiteboard: [webcompat:p2][wptsync upstream])

Attachments

(2 files)

Bug 1149357: Make nsImageFrame::mIntrinsicSize account for density.
(deleted), text/x-review-board-request
dholbert
: review+
Details
Bug 1149357: Properly update responsive images for density changes.
(deleted), text/x-review-board-request
dholbert
: review+
Details 
When an image lacks a width and height, strange padding appears next to the image when using flexbox or float.  

Please see:
https://jsfiddle.net/o6wv5ju7/
Depends on: srcset
Looks like https://www.webkit.org/demos/srcset/image-2x.png is an 800px by 800px image.  So we should end up with an <img> that's 400px wide if it's doing the 2x thing, right?

That's how wide the nsImageFrame ends up, but its container is 800px wide, which suggests we're somehow ignoring the 2x thing when computing the intrinsic size of the image.

As far as I can tell, intrinsic size calculation (in nsImageFrame) calls GetIntrinsicSize() on the imgIContainer, while nsImageFrame::ComputeSize ends up calling GetWidth() on the same imgIContainer.  But I'd think those would match.
Component: DOM → Layout: Images
Flags: needinfo?(seth)
Summary: Not setting a width and height on srcset image causes strange padding → Not setting a width and height on srcset image causes incorrect width for shrink-wrapping ancestor
John, can you take a look at this?
Flags: needinfo?(seth) → needinfo?(john)
This is probably because only nsImageFrame::ComputeSize takes into account srcset (by calling imageLoader->GetNaturalWidth/Height), everywhere else just looks straight at the image (GetMinISize, GetPrefISize, GetIntrinsicSize, GetIntrinsicSize). And then there are the nsImageFrame specific uses of the intrinsic size too.
Probably on my plate instead.
Flags: needinfo?(john) → needinfo?(josh)
I'm seeing a similar issue and was wondering if it was the same as this bug or do I need to open another one.

On: http://home.brandthunder.com/nfltexans/

The twitter, facebook, instagram, etc. icons are using srcset.

With dom.image.srcset.enabled set to false, they lay out correctly.

With dom.image.srcset.enabled set to true, they get a lot of extra padding on either side.
Also issues parents with inline-block: Incorrect width for elements with srcset image child in 2x mode

http://codepen.io/anon/pen/pJOOaN
Another example of this issue: http://jsfiddle.net/ye7oo5Lg/1/
(JSFiddle example must be viewed on a high-res display to see the problem)
Any updates about issue?

Somebody knows workaround for fix issue on old Firefox version?
tnikkel's comment suggests that there's a bunch of layout knowledge that I don't have which would be useful to solve this bug. I don't think waiting on me to acquire that knowledge is a winning strategy, so let's not pretend that I'm planning to work on this any longer.
Flags: needinfo?(josh)
I think this is quite critical bug, since no ways for user-side workaround.

But as I understood it's easy to fix in core code (I'm not a developer).

What are the chances that someone from Mozilla developers team will pay attention to this problem?
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> This is probably because only nsImageFrame::ComputeSize takes into account
> srcset (by calling imageLoader->GetNaturalWidth/Height), everywhere else
> just looks straight at the image (GetMinISize, GetPrefISize,
> GetIntrinsicSize, GetIntrinsicSize). And then there are the nsImageFrame
> specific uses of the intrinsic size too.

When I implemented this, I was told ComputeSize should be sufficient for layout purposes, and the other users in that class were concerned about the size of their source image, rather than the layout-purposes "intrinsic size". Clearly that's not the case everywhere -- do you have any pointers on which users in nsImageFrame need to be using the scaled size? It's rather opaque what most of the values in there get used for.
Flags: needinfo?(tnikkel)
landy2005, people are paying attention to this problem; it was on my list of things to look at once I saw comment 12, for example.  But we all have other things that need doing too.

I have no idea what gave you the idea that this is "easy to fix"; comment 3 and comment 14 should make it clear that it's not for most people.

John, ComputeSize() is what computes the actual size of the frame, but the GetMin/PrefISize functions are what determines the size of its _parent_, which is what this bug is about.  That was certainly the case when srcset was implemented, so either the question or the answer about layout purposes was somewhat misunderstood...

As far as what should be using the scaled size... One issue is that I seem to recall that GetNaturalWidth in the srcset case sometimes depends on layout.  Or does it just depend on the size of the viewport?  If it's just the size of the viewport, we're still going to have to clear our some cached intrinsic sizes when the viewport size changes, which is going to be exciting.  If it depends on layout, then I have no idea how this is even expected to work.  

On the assumption that it's just viewport-dependent, the uses of mIntrinsic* in nsImageFrame seem to be as follows:

1)  GetSourceToDestTransform.  This looks like part of the object-fit stuff,
    but I'm not sure what the transform is used for; this might want the actual
    intrinsic size of the image.  Though the docs for ComputeObjectDestRect don't
    sound that way...
2)  PredictedDestRect.  I guess the same?  It's also calling ComputeObjectDestRect.
3)  ComputeSize, which already handles srcset.
4)  GetMinISize/GetPrefISize.  These should take srcset stuff into account.
5)  GetIntrinsicSize.  Not sure about this one.  In practice it may not
    matter: the nsLineLayout consumer only cares about whether there is
    a size at all, the VectorImage and nsSubDocumentFrame consumers don't
    ever call this on an nsImageFrame, I expect, and in the ImageDocument 
    case there is no srcset.  The lack of documentation for what
    GetIntrinsicSize is supposed to do doesn't really help.
6)  GetIntrinsicRatio.  Is this even affected by srcset stuff?  If it is,
    then you probably want the srcset-affected thing here at least in the flexbox
    caller.
7)  PaintImage, calling ComputeObjectDestRect.  This should likely take
    the actual image intrinsic size.
8)  GetIntrinsicImageSize.  This one should use the srcset-affected size for
    the reflow state consumer, at least.

I think that's all of them.  Seth, does the above sound right?  What should items 1 and 2 do?  Do you have time to work on this bug in general?
Flags: needinfo?(seth)
Ok, maybe it's not so "simple to fix", but as I see no one from mozilla team wants to deal with the problem.
Functionality added without any verification, just for was possible to tell that "we have it".

After last comment released 2 FF versions, there have been no changes or comments .. again. Will wait for next two releases.
(In reply to landy2005 from comment #13)
> I think this is quite critical bug, since no ways for user-side workaround.
> 
> But as I understood it's easy to fix in core code (I'm not a developer).
> 
> What are the chances that someone from Mozilla developers team will pay
> attention to this problem?

I use next workaround - just set width or height to image
Example <img src="..." srcset="... 1x, ... 2x" style="width: _width of 1x_px;">
(In reply to Elnur Kurtaliev from comment #17)
> (In reply to landy2005 from comment #13)
> > I think this is quite critical bug, since no ways for user-side workaround.
> > 
> > But as I understood it's easy to fix in core code (I'm not a developer).
> > 
> > What are the chances that someone from Mozilla developers team will pay
> > attention to this problem?
> 
> I use next workaround - just set width or height to image
> Example <img src="..." srcset="... 1x, ... 2x" style="width: _width of
> 1x_px;">

Tnx, good workaround, but only when the sizes are in advance known.
Not my case (html/css code generated on server, img tags has links to script, which generate image(s) on user request and can be in any casual size).
Flags: needinfo?(bugs)
(In reply to Boris Zbarsky [:bz] from comment #15)
> John, ComputeSize() is what computes the actual size of the frame, but the
> GetMin/PrefISize functions are what determines the size of its _parent_,
> which is what this bug is about.  That was certainly the case when srcset
> was implemented, so either the question or the answer about layout purposes
> was somewhat misunderstood...

"Clearly that's not the case everywhere".  As in, yes, I was mistaken.  I don't harbor any secret grudge against shrink-wrapping ancestors.  That I would admit to.

> As far as what should be using the scaled size... One issue is that I seem
> to recall that GetNaturalWidth in the srcset case sometimes depends on
> layout.  Or does it just depend on the size of the viewport?  If it's just
> the size of the viewport, we're still going to have to clear our some cached
> intrinsic sizes when the viewport size changes, which is going to be
> exciting.  If it depends on layout, then I have no idea how this is even
> expected to work.  

I believe it can depend on viewport size only - e.g. <srcset sizes="100vw">.  Percent values are explicitly disallowed.  An explicit goal of the spec was that the intrinsic size and thus the optimal image source can be selected immediately without layout/onload dependencies.

> 6)  GetIntrinsicRatio.  Is this even affected by srcset stuff?  If it is,
>     then you probably want the srcset-affected thing here at least in the
> flexbox
>     caller.

This may have changed in newer versions of the picture spec, but I believe there is no way to adjust ratio -- "sizes" affects width only, and density scaling should preserve ratio.
(In reply to John Schoenick [:johns] from comment #19)
> I believe it can depend on viewport size only - e.g. <srcset sizes="100vw">.
> Percent values are explicitly disallowed.  An explicit goal of the spec was
> that the intrinsic size and thus the optimal image source can be selected
> immediately without layout/onload dependencies.

Right, however, sometimes viewport size can depend on layout -- if you're in an iframe that hasn't been laid out yet.

> This may have changed in newer versions of the picture spec, but I believe
> there is no way to adjust ratio -- "sizes" affects width only, and density
> scaling should preserve ratio.

This is still the case.
> Right, however, sometimes viewport size can depend on layout

Right, but not on the layout of things inside that viewport.
Any progress news?
FF 49 issue still there..
Just so someone who has the time to deal with the fallout can pick this up as needed, here are the things that need to be done here:

1)  Viewport resize needs to clear cached intrinsic sizes that depend
    on scrset-derived intrinsic size, or maybe only on
    viewport-dependent-srcset-derived intrinsic size.  Doing
    this without performance degradation for resizes will be quite exciting.
    It's worth checking how other browsers behave in these viewport-size-dependent
    srcset situations with intrinsic sizing.  Maybe we can just have image frames
    that have viewport-size-dependent sizing register themselves with the
    viewport and it can clear cached intrinsics up their ancestor chains on
    resize.  But want to avoid doing multiple redundant walks up a deep frame tree...
2)  Once item #1 is done, go through the list in comment 15 and sort out which of those
    callsites care about the srcset-influenced intrinsic size, fixing them accordingly.
Blocks: 1310203, 1251327
Just to add some more data to it, the bug exists even in these scenarios:

- img height and width attribute are set in the tag, but in CSS img { width: initial; height: initial; } are used. Basically overwriting via CSS also triggers the bug.
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox57: --- → affected
Priority: -- → P2
Whiteboard: [webcompat]
Any news?
Latest version is still affected by this.
Flags: webcompat?
Blocks: 1400410
Testcase from duplicate bug: https://bugzilla.mozilla.org/attachment.cgi?id=8913355

EXPECTED RESULTS:
 No red should be visible.

ACTUAL RESULTS:
 Red is visible.
URL: https://bugzilla.mozilla.org/attachme...
Flags: needinfo?(bmo)
Flags: needinfo?(bmo)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> Just so someone who has the time to deal with the fallout can pick this up
> as needed, here are the things that need to be done here:
> 
> 1)  Viewport resize needs to clear cached intrinsic sizes that depend
>     on srcset-derived intrinsic size, or maybe only on
>     viewport-dependent-srcset-derived intrinsic size.  Doing
>     this without performance degradation for resizes will be quite exciting.
>     It's worth checking how other browsers behave in these viewport-size-dependent
>     srcset situations with intrinsic sizing.  Maybe we can just have image frames
>     that have viewport-size-dependent sizing register themselves with the
>     viewport and it can clear cached intrinsics up their ancestor chains on
>     resize.  But want to avoid doing multiple redundant walks up a deep frame tree...

I'd note that you could use an approach like the old nsMediaQueryResultCacheKey that only does the invalidation when the choice of image (or even the size) changes, rather than doing the invalidation whenever the viewport changes.

I'd also wager this may be substantially lower priority than fixing (2), though it's still worth doing.
I think this is a high priority to get assigned.  (Should it be on the webcompat:p1 list?  What are the criteria for that?)
Flags: needinfo?(past)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #35)
> I think this is a high priority to get assigned.  (Should it be on the
> webcompat:p1 list?  What are the criteria for that?)

I'll defer to Mike on this.
Flags: needinfo?(past) → needinfo?(miket)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #35)
> I think this is a high priority to get assigned.  (Should it be on the
> webcompat:p1 list?  What are the criteria for that?)

For now, the criteria is "causes a lot of known bustage" or "affects Google Tier 1 search". Let's set this to [webcompat:p2] for now because it does seem important.
Flags: webcompat?
Flags: webcompat+
Flags: needinfo?(miket)
Whiteboard: [webcompat] → [webcompat:p2]
I plan to take a look at bug 215083 which means I'm going to deal with a bunch of this code, so I'll read through and try to keep this in mind.
Flags: needinfo?(emilio)
I know why this is. Patch posted to bug 215083 fixes this. Will submit the fix separately though, just for the sake of being less risky and more self-contained.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
The way I structured the patches makes this block bug 215083. Clearing a few old needinfo?s which I suspect are not relevant any longer.
Blocks: content-url-element
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth.bugzilla)
Flags: needinfo?(bugs)
Actually I suspect we can simplify this even further removing the scaling from ComputeSize altogether, given it was introduced for srcset...
Emilio, how does that change address comment 24 item 1?

For that matter, have you audited the list from comment 15 to see what concept of "intrinsic size" those consumers want?
Flags: needinfo?(emilio)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #45)
> Emilio, how does that change address comment 24 item 1?

It does not. I think it's worth considering doing that as a followup, since it may be nontrivial, and it's clearly not a regression. If you think it should block it though, let me know, as per below I don't think it'll be terribly hard. But I need to write tests :).

Also note that it has to be notified of all media-dependent changes, because you can stash random media expressions in the SourceSizeList. However it looks to me that we already notify responsive content on media feature changes to queue an image load task, so shouldn't be a big deal to clear intrinsic sizes on the frame from HTMLImageElement::MediaFeatureValuesChanged if there's a responsive image selector (we could even pass a bit more state, like the MediaFeatureChange from the caller, and do it only in cases that matter).

> For that matter, have you audited the list from comment 15 to see what
> concept of "intrinsic size" those consumers want?

I did audit the consumers of mIntrinsicSize these days, and I think they all want the actual srcset-influenced intrinsic size.
Flags: needinfo?(emilio)
> and it's clearly not a regression.

Well, sort of.  Right now our layout is consistent (though sometimes consistently wrong).  With the proposed change but without addressing comment 24 item 1, the layout would be correct sometimes and wrong other times depending on exactly how things get ordered, right?

I'm probably OK with that work being in a followup as long as we make sure that followup lands in the same release cycle as this fix.

> I did audit the consumers of mIntrinsicSize these days

Great, thank you for doing that.  I didn't see a description of the audit in the commit message; hence the question.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #47)
> > and it's clearly not a regression.
> 
> Well, sort of.  Right now our layout is consistent (though sometimes
> consistently wrong).  With the proposed change but without addressing
> comment 24 item 1, the layout would be correct sometimes and wrong other
> times depending on exactly how things get ordered, right?
> 
> I'm probably OK with that work being in a followup as long as we make sure
> that followup lands in the same release cycle as this fix.

It's unclear to me this is a problem... We do check for mCurrentDensity changes from MediaFeatureValuesChanged, and that ends up triggering a new image request via LoadImage.

That ends up in nsImageFrame::NotifyNewCurrentRequest, which will reset mIntrinsicSize / mIntrinsicRatio if the size is not yet available, or update it otherwise.

That being said, that looks somewhat inefficient if the source chosen didn't actually change and only the density changed. That short-circuiting was implemented in bug 1268182, btw.
> We do check for mCurrentDensity changes from MediaFeatureValuesChanged, and that
> ends up triggering a new image request via LoadImage.

Hmm.  And the intrinsic size can't change without doing this new load?  If so, great.  Let's just make sure we have tests.
Flags: needinfo?(bzbarsky)
Comment on attachment 8976458 [details]
Bug 1149357: Make nsImageFrame::mIntrinsicSize account for density.

https://reviewboard.mozilla.org/r/244582/#review251880

::: commit-message-1800b:1
(Diff revision 2)
> +Bug 1149357: Make mIntrinsicSize account for density. r?dholbert

optional suggestion: Maybe give the full name of the variable here ("nsImageFrame::mIntrinsicSize")?

(to make it clearer at-a-glance what general area of code is being changed here, to assist developers in the future who may be skimming past this in a pushlog & wanting to know at a glance whether it is/isn't related to whatever their regression is)

::: dom/html/HTMLImageElement.h:46
(Diff revision 2)
> +  ResponsiveImageSelector* GetResponsiveImageSelector()
> +  {

Can you add "const" at the beginning & end of the signature here, to avoid leaking too much data / mutability?  e.g. I think this should be:
  const ResponsiveImageSelector* GetResponsiveImageSelector() const

(See notes below about the callsite - I think we need to tweak that & one other API to be able to do this.)

::: layout/generic/nsImageFrame.cpp:299
(Diff revision 2)
> +static void
> +AdjustIntrinsicSizeIfNeeded(nsIContent& aContent, nsSize& aSize)
> +{

This is perhaps too generic of a name... It should hint that it's a scale & that it's density/DPI-related.  How about:
"ScaleIntrinsicSizeForDensity"

::: layout/generic/nsImageFrame.cpp:307
(Diff revision 2)
> +  ResponsiveImageSelector* selector = image->GetResponsiveImageSelector();
> +  if (!selector) {
> +    return;
> +  }
> +
> +  double density = selector->GetSelectedImageDensity();

Could you declare selector as "const ResponsiveImageSelector*"? (related to other "const" suggestion above)

To do that, I think we need to *also* annotate that class's GetSelectedImageDensity function as "const", and that's probably a good idea anyway. (and I don't think this const rabbit-hole goes any deeper than that...)

::: layout/generic/nsImageFrame.cpp:314
(Diff revision 2)
> +  if (aSize.width != -1) {
> +    aSize.width = NSToIntRound(double(aSize.width) / density);
> +  }
> +  if (aSize.height != -1) {
> +    aSize.height = NSToIntRound(double(aSize.height) / density);
> +  }

Two things:
 (1) For the density==1.0 scenario (which I expect is common), it'd probably be worth skipping the double conversion + division + rounding + etc. since no scaling is necessary.  So maybe add this before the adjustments, to avoid unnecessary math for that common case:
  if (density == 1.0) {
    return;
  }

 (2) These aSize components have type "nscoord", so this should be NSToCoordRound, rather than NSToIntRound.

::: layout/reftests/image/image-srcset-isize.html:24
(Diff revision 2)
> +  window.addEventListener("load", function() {
> +    setTimeout(function() {
> +      var img = document.querySelector("img");
> +      img.onload = clearWait;
> +      img.onerror = clearWait;
> +      img.src = img.src;
> +    }, 0);

* Why do we need setTimeout(...,0) here, out of curiosity?  It's more reliable to hook into either "load" or "MozReftestInvalidate" rather than to one of those plus a setTimeout.  (If load wasn't sufficient, I suspect you want MozReftestInvalidate?)

* Why are we doing img.src = img.src? img.src is the empty string here, I think? So we're setting it from empty-string to empty-string, which feels like it is (or could be optimized to) a no-op...  I'm not clear what that assignment is aiming to achieve.

::: layout/reftests/image/reftest.list:129
(Diff revision 2)
>  == image-srcset-svg-2x.html image-srcset-svg-2x-ref.html
>  == image-srcset-svg-1x.html image-srcset-svg-1x-ref.html
>  == image-srcset-svg-default-2x.html image-srcset-svg-default-2x-ref.html
>  == image-srcset-svg-default-1x.html image-srcset-svg-default-1x-ref.html
>  
> +== image-srcset-isize.html image-srcset-isize-ref.html

Nit: this should be inserted up a bit, in the alphabetical spot within the other img-srcset tests (right after image-srcset-default-src-1x.html I think)

Also, please be sure we've got a testcase along the lines of what bz is requesting in comment 49.
Comment on attachment 8976458 [details]
Bug 1149357: Make nsImageFrame::mIntrinsicSize account for density.

https://reviewboard.mozilla.org/r/244582/#review251880

> optional suggestion: Maybe give the full name of the variable here ("nsImageFrame::mIntrinsicSize")?
> 
> (to make it clearer at-a-glance what general area of code is being changed here, to assist developers in the future who may be skimming past this in a pushlog & wanting to know at a glance whether it is/isn't related to whatever their regression is)

Sounds good, will ammend.

> Can you add "const" at the beginning & end of the signature here, to avoid leaking too much data / mutability?  e.g. I think this should be:
>   const ResponsiveImageSelector* GetResponsiveImageSelector() const
> 
> (See notes below about the callsite - I think we need to tweak that & one other API to be able to do this.)

I cannot, because ResponsiveImageSelector has internal caching... I tried before submitting the patch as-is.

I could mark mSelectedCandidateIndex as `mutable` I guess, but I like that even less I think...

> This is perhaps too generic of a name... It should hint that it's a scale & that it's density/DPI-related.  How about:
> "ScaleIntrinsicSizeForDensity"

SGTM

> Could you declare selector as "const ResponsiveImageSelector*"? (related to other "const" suggestion above)
> 
> To do that, I think we need to *also* annotate that class's GetSelectedImageDensity function as "const", and that's probably a good idea anyway. (and I don't think this const rabbit-hole goes any deeper than that...)

Same issue as above unfortunately.

> Two things:
>  (1) For the density==1.0 scenario (which I expect is common), it'd probably be worth skipping the double conversion + division + rounding + etc. since no scaling is necessary.  So maybe add this before the adjustments, to avoid unnecessary math for that common case:
>   if (density == 1.0) {
>     return;
>   }
> 
>  (2) These aSize components have type "nscoord", so this should be NSToCoordRound, rather than NSToIntRound.

Sounds good.

> * Why do we need setTimeout(...,0) here, out of curiosity?  It's more reliable to hook into either "load" or "MozReftestInvalidate" rather than to one of those plus a setTimeout.  (If load wasn't sufficient, I suspect you want MozReftestInvalidate?)
> 
> * Why are we doing img.src = img.src? img.src is the empty string here, I think? So we're setting it from empty-string to empty-string, which feels like it is (or could be optimized to) a no-op...  I'm not clear what that assignment is aiming to achieve.

I was copying the format of existing tests, see all the srcset- tests. This is because reftest-zoom applies onload, which means that we need reftest-wait instead of the normal reftest mechanism. Not sure why the setTimeout(.., 0).

The img.src bits are, I think, because the image could've loaded already by the time the timeout runs, so I assume that it guarantees error / load events to trigger again at least once afterwards, so the test doesn't timeout.

> Nit: this should be inserted up a bit, in the alphabetical spot within the other img-srcset tests (right after image-srcset-default-src-1x.html I think)
> 
> Also, please be sure we've got a testcase along the lines of what bz is requesting in comment 49.

Argh, it's certainly annoying that reftest-zoom waits for onload in this way, otherwise we could remove all this awkwardness, and the test-cases would be perfect for bz's purposes ;).

I'll figure out how to test this with the sizes attributes and an iframe or something something.
Oh, great... We have literally 0 occurrences of "sizes=" in layout/reftests... This is gonna be fun...
And of course my patch does break some stuff, like:

  <iframe onload="this.width = '500'" width="200" height="500" srcdoc='<!doctype html><img srcset="/images/green-256x256.png 100w" style="max-width: 100%" sizes="(min-width: 400px) 10px, 20px">'></iframe>

Fixing...
Comment on attachment 8976458 [details]
Bug 1149357: Make nsImageFrame::mIntrinsicSize account for density.

https://reviewboard.mozilla.org/r/244582/#review252388
Attachment #8976458 - Flags: review?(dholbert) → review+
Comment on attachment 8979921 [details]
Bug 1149357: Properly update responsive images for density changes.

https://reviewboard.mozilla.org/r/246108/#review252384

::: commit-message-980a8:3
(Diff revision 1)
> +Before that we were relying on the reflow the viewport resize caused to get the
> +new density. Also mCurrentDensity got out of sync as well.
> +
> +This was generally unsound, since you can stash random media in a sizes
> +attribute.
> +
> +MozReview-Commit-ID: Eqy16ygHRLo

Could you provide an explanation for what's changing & why here? Right now the commit message just that something was wrong beforehand, but it doesn't really talk about what the patch is changing & how things will be working now.  A few more comments sprinkled through the code might be helpful with this, too...

(This might be more obvious if I were familiar with how responsive image handling works in Gecko right now, but I'm not really [you might be the only person who is at this point :)], so I stared at this for a bit but it's not entirely obvious to me what's going on.)

::: dom/html/HTMLImageElement.cpp:969
(Diff revision 1)
> +  // In the case we actually trigger a new load, the code will end up in the
> +  // image frame via NotifyNewCurrentRequest.
> +  auto UpdateDensityOnly = [&]() -> void {
> +    if (mCurrentDensity == currentDensity) {

The comment wording here feels a bit confusing/awkward ("the code will end up in the image frame").

I think you mean to say something like the following, maybe:
 "...the image frame will find out about it when it receives a call to NotifyNewCurrentRequest()"

I'm also not 100% sure what the connection is between this comment and the code that immediately follows it (UpdateDensityOnly) -- could you make that clearer, or perhaps move the comment closer to whatever it's documenting?
Attachment #8979921 - Flags: review?(dholbert) → review-
Comment on attachment 8979921 [details]
Bug 1149357: Properly update responsive images for density changes.

https://reviewboard.mozilla.org/r/246108/#review252384

> Could you provide an explanation for what's changing & why here? Right now the commit message just that something was wrong beforehand, but it doesn't really talk about what the patch is changing & how things will be working now.  A few more comments sprinkled through the code might be helpful with this, too...
> 
> (This might be more obvious if I were familiar with how responsive image handling works in Gecko right now, but I'm not really [you might be the only person who is at this point :)], so I stared at this for a bit but it's not entirely obvious to me what's going on.)

Hopefully the new commit message and https://github.com/whatwg/html/issues/3709 satisfies you :)

> The comment wording here feels a bit confusing/awkward ("the code will end up in the image frame").
> 
> I think you mean to say something like the following, maybe:
>  "...the image frame will find out about it when it receives a call to NotifyNewCurrentRequest()"
> 
> I'm also not 100% sure what the connection is between this comment and the code that immediately follows it (UpdateDensityOnly) -- could you make that clearer, or perhaps move the comment closer to whatever it's documenting?

Yeah, I wanted to point out the code that runs when this path is _not_ taken, hopefully now it's clearer.
Comment on attachment 8979921 [details]
Bug 1149357: Properly update responsive images for density changes.

https://reviewboard.mozilla.org/r/246108/#review252708

::: dom/html/HTMLImageElement.cpp:969
(Diff revision 2)
> +  // Update state when only density may have changed (i.e., the source to load
> +  // hasn't changed, and we don't do any request at all). We need (apart from

The wording right now ("Update state when...") sounds like *that is what we are doing when we hit this comment*.  But really, you're intending to document what the following helper-function is for (though that helper function may or may not even be run).

How about:
 // Helper function to update state [...]
or something like that?

::: dom/html/HTMLImageElement.cpp:974
(Diff revision 2)
> +  // In the case we actually trigger a new load, the code will end up in
> +  // nsImageFrame::NotifyNewCurrentRequest, which takes care of that for us.
> +  auto UpdateDensityOnly = [&]() -> void {

"the code will end up in" still feels a bit hand-wavy.

How about:
  // In the case we actually trigger a new load, that load will trigger a call
  // to nsImageFrame::NotifyNewCurrentRequest, which takes care of that for us.

(assuming this is roughly correct :) )

::: dom/html/HTMLImageElement.cpp:1003
(Diff revision 2)
>    if (mResponsiveSelector) {
>      nsCOMPtr<nsIURI> url = mResponsiveSelector->GetSelectedImageURL();
>      nsCOMPtr<nsIPrincipal> triggeringPrincipal = mResponsiveSelector->GetSelectedImageTriggeringPrincipal();
>      selectedSource = url;
> -    currentDensity = mResponsiveSelector->GetSelectedImageDensity();
> -    if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource, currentDensity)) {
> +    if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource)) {
> +      UpdateDensityOnly();

Previously we were looking up "currentDensity" here; now we are not. Is that a problem?

(If aForce is false, we will have skipped the higher-up clause, and so currentDensity will still be at 1.0 -- whereas previously, we would've looked it up directly here via a call to "currentDensity = mResponsiveSelector->GetSelectedImageDensity()" that you're removing...)

::: layout/generic/nsImageFrame.cpp:723
(Diff revision 2)
> +  PresShell()->FrameNeedsReflow(
> +    this, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);

The linewrap/deindentation here doesn't really match canonical Mozilla style & isn't really necessary. Can we just unwrap this to...
  PresShell()->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
                                NS_FRAME_IS_DIRTY);
...or similar?
Attachment #8979921 - Flags: review?(dholbert) → review+
Comment on attachment 8979921 [details]
Bug 1149357: Properly update responsive images for density changes.

https://reviewboard.mozilla.org/r/246108/#review252708

> The wording right now ("Update state when...") sounds like *that is what we are doing when we hit this comment*.  But really, you're intending to document what the following helper-function is for (though that helper function may or may not even be run).
> 
> How about:
>  // Helper function to update state [...]
> or something like that?

Yup, sounds great :)

> Previously we were looking up "currentDensity" here; now we are not. Is that a problem?
> 
> (If aForce is false, we will have skipped the higher-up clause, and so currentDensity will still be at 1.0 -- whereas previously, we would've looked it up directly here via a call to "currentDensity = mResponsiveSelector->GetSelectedImageDensity()" that you're removing...)

It is not, because aForce is always true from QueueImageLoadTask, which is what all the responsive image stuff uses... But I agree that lacking a cleaner model for this is worth looking at it in the !aForce case too... Because the argument names don't even make that much sense / are clear at all... This code could get some more love I think.

> The linewrap/deindentation here doesn't really match canonical Mozilla style & isn't really necessary. Can we just unwrap this to...
>   PresShell()->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
>                                 NS_FRAME_IS_DIRTY);
> ...or similar?

Fixed.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19bef57314b
Make nsImageFrame::mIntrinsicSize account for density. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d94b092d7e0
Properly update responsive images for density changes. r=dholbert
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11162 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p2] → [webcompat:p2][wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/d19bef57314b
https://hg.mozilla.org/mozilla-central/rev/2d94b092d7e0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1469000
No longer depends on: 1469000
Depends on: 1469000
Depends on: 1469784
Managed to check and reproduce the bug using the test mentioned in comment 31.
Was able to reproduce using Firefox 50.0a1 (2016-07-27) on Win 10x64.

Have checked with Firefox 62.0b12  on Win10x64, Ubuntu 16.04LTS, macOS 10.13.4 and can confirm the issue is no longer reproducible.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
Product: Core → Core Graveyard
Product: Core Graveyard → Core
Depends on: 1490685
Regressions: 1704204
Duplicate of this bug: 1231355
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: