If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 26

Status

()

Core
Layout: Images
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla27
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 808101 [details] [diff] [review]
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.
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)

Also,
  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 ;-)
(Assignee)

Comment 3

4 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.
(Assignee)

Comment 4

4 years ago
Created attachment 808202 [details] [diff] [review]
patch v2
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+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bd43aa9a018
(Assignee)

Comment 7

4 years ago
Bug 847223 added this code, so adding dependency to track that.
Blocks: 847223
https://hg.mozilla.org/mozilla-central/rev/9bd43aa9a018
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Comment 9

4 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+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3c9e1256c65
status-firefox26: --- → fixed
status-firefox27: --- → fixed
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? → ---

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.