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

RESOLVED FIXED in Firefox 26



6 years ago
9 months ago


(Reporter: tnikkel, Assigned: tnikkel)


Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)


(Whiteboard: [qa-])


(1 attachment, 1 obsolete attachment)



6 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #808101 - Flags: review?(matspal)
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)

  transformedRect.x += scrollPort.x - transformedRect.x;
is the same as:
  transformedRect.x = scrollPort.x;
Er, std::min that is... bah, reviewing while having morning coffee ;-)

Comment 3

6 years ago
(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

6 years ago
Posted patch patch v2Splinter Review
Attachment #808101 - Attachment is obsolete: true
Attachment #808101 - Flags: review?(matspal)
Attachment #808202 - Flags: review?(matspal)
Comment on attachment 808202 [details] [diff] [review]
patch v2

OK, looks good.  r=mats
Attachment #808202 - Flags: review?(matspal) → review+

Comment 7

6 years ago
Bug 847223 added this code, so adding dependency to track that.
Blocks: 847223
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 9

6 years ago
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
Attachment #808202 - Flags: approval-mozilla-aurora?
blocking-b2g: --- → koi?
Attachment #808202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
blocking-b2g: koi? → ---
Whiteboard: [qa-]


9 months ago
Product: Core → Core Graveyard


9 months ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.