919129 - fix nsLayoutUtils::UpdateImageVisibilityForFrame to not expand the image rect too much

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

nsLayoutUtils::UpdateImageVisibilityForFrame expands the image rect to the scrollport of the scrollframe for each scroll frame it encounters. This is way too much.

The rect of the image may be scrolled out of view in the scroll frame. Using a rect that is scrolled out of view for further visibility calculations seems wrong. Instead pretend the rect was scrolled into view.

Comment 1

[Mats Palmgren (inactive)]

This makes sense for a small image (relative the scroll port), but it seems overly pessimistic
for an image that's larger than the scroll port.  I think we want to clip the result to the
scroll port after the lines you added.  (which, given that x,y is now inside the scroll port
with the lines you added, boils down to std::max of the two widths/heights)

Also,
  transformedRect.x += scrollPort.x - transformedRect.x;
is the same as:
  transformedRect.x = scrollPort.x;

Comment 2

[Mats Palmgren (inactive)]

Er, std::min that is... bah, reviewing while having morning coffee ;-)

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #1)
> I think we want to clip the result to the
> scroll port after the lines you added.

Yes of course. This was what I always intended to do but forget to add after I had the shifting part done.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 808202 [details] [diff] [review]
patch v2

OK, looks good.  r=mats

Comment 6

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd43aa9a018

Comment 7

[Timothy Nikkel (:tnikkel)]

Bug 847223 added this code, so adding dependency to track that.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9bd43aa9a018

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 808202 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 847223
User impact if declined: this patch just fixes the code that was added in bug 847223 to determine if a image is visible or not. it stops images from being considered visible or nearly visible when they are not. taking this patch means we only have one version of this code around and never ship the sub-optimal code.
Testing completed (on m-c, etc.): been on m-c for a while now, no problems found
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: none

Comment 10

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3c9e1256c65

Comment 11

[Jason Smith [:jsmith]]

We probably don't need to bother triaging this for koi at this point, given that this is already landed on aurora per comment 10.