incorrect image repainting on zoom when scrollbar values change

VERIFIED FIXED in mozilla1.9.3a1

Status

()

VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: jfkthame, Assigned: roc)

Tracking

Trunk
mozilla1.9.3a1
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 410618 [details]
screenshot showing incorrectly-painted image

When zooming in on an image, it will repaint incorrectly if the zoom operation also causes the window's scrollbar positions to change; parts of the unzoomed image are left.

Steps to reproduce:

* Open https://bug521531.bugzilla.mozilla.org/attachment.cgi?id=410501 in a new window or tab.

* Size the window so that the title bar indicates that the image (1058x811 pixels) is scaled to 60%.

* Move the mouse pointer (which should show as a (+) magnifier) to the "T" at the beginning of the text "Thanks for supporting..." in the image.

* Click to zoom in.

The result is shown in the attachment; note the "stripe" of unzoomed content in the middle of the image, and also at the left edge.

The effect appears to be related to the fact that the scroll position of the window content is changed by the zoom operation (in order to center on the mouse-click position); clicking near the top left, so that the image stays anchored to the corner, does not show the problem.

This behavior is happening on trunk, but does not occur in 3.5.4.
Assignee: nobody → roc
Flags: blocking1.9.2?
Works on Linux. I think this is bug in nsChildView::Scroll where we don't move the invalid region correctly when we scroll.
I think this is a regression from bug 517802.

When we call translateRectsNeedingDisplayInRect, we pass 'rect', which is the source rect of the copy. But Apple's documentation says:
http://developer.apple.com/mac/library/documentation/Cocoa/Reference/ApplicationKit/Classes/NSView_Class/Reference/NSView.html#//apple_ref/occ/instm/NSView/translateRectsNeedingDisplayInRect:by:

# Clears all dirty rectangles in the intersection of clipRect and the view's bounds.
# Shifts the retrieved rectangles by the delta offset.
# Clips the result to the intersection of clipRect and the view's bounds

We actually don't want the last step to happen here. Part of the destination rect will be outside the source rect but we still need invalid rectangles to be moved there.
Blocks: 517802
Maybe it wasn't a regression from bug 517802 per se, but an incomplete fix there.
Created attachment 410687 [details] [diff] [review]
fix

This fixes it.
Attachment #410687 - Flags: review?(mstange)
Definitely a widget bug, and I'm taking the liberty of marking it blocking, since it could affect all kinds of Mac scrolling.
Component: Layout: Images → Widget: Cocoa
Flags: blocking1.9.2? → blocking1.9.2+
QA Contact: layout.images → cocoa
Whiteboard: [needs review]
Duplicate of this bug: 522708
Comment on attachment 410687 [details] [diff] [review]
fix

Thanks for figuring this out!
Attachment #410687 - Flags: review?(mstange) → review+
Status: NEW → ASSIGNED
Whiteboard: [needs review] → [needs landing]
Actually I suspect this patch may not be correct. For example, if there's a box in the middle of the content that is not being scrolled, and it has pending invalidates, this patch will cause the invalid area to be moved even though they shouldn't be. I don't see how to do what we need using this translateRectsIfNeeded API.

I think one way to work around it would be to invalidate the area that's 'all' minus the union of the destination rects, if viewWasDirty is true.
Created attachment 411115 [details] [diff] [review]
Create nsIntRegion

To make this code more convenient, I need nsIntRegion. I'm planning to use this for layers anyhow, and it will probably also be useful in other places. For now we can just delegate the entire implementation to nsRegion.
Attachment #411115 - Flags: review?(jmuizelaar)
Created attachment 411116 [details] [diff] [review]
fix v2

Fixes the bug I noticed in the above comment.
Attachment #410687 - Attachment is obsolete: true
Attachment #411116 - Flags: review?(mstange)
Whiteboard: [needs landing] → [needs review]
(In reply to comment #9)
> Created an attachment (id=411115) [details]
> Create nsIntRegion

Did you attach the wrong patch here? This attachment is the same as 'fix v2'.
Created attachment 411137 [details] [diff] [review]
nsIntRegion

Indeed, I stuffed it up
Attachment #411115 - Attachment is obsolete: true
Attachment #411137 - Flags: review?(jmuizelaar)
Attachment #411115 - Flags: review?(jmuizelaar)
(In reply to comment #8)
> Actually I suspect this patch may not be correct. For example, if there's a box
> in the middle of the content that is not being scrolled, and it has pending
> invalidates, this patch will cause the invalid area to be moved even though
> they shouldn't be.

Isn't that exactly what the first comment in the patch says? Or is this a different situation? I'm confused.
In other words, why do we first say "it doesn't matter" and then work around it anyway?
Oops! That comment is incorrect and should be removed. I left it in by mistake.

The truth is that there are situations where Gecko will not repaint areas that are inside the bounding-box of the scroll rects. One example is the example I gave in comment #8.
Comment on attachment 411116 [details] [diff] [review]
fix v2

I see.

>+           (invalidate = iter.Next()) != nsnull;) {

The "!= nsnull" is unnecessary, no?
Attachment #411116 - Flags: review?(mstange) → review+
Yes, but I think it's clearer.
Comment on attachment 411137 [details] [diff] [review]
nsIntRegion

Seems fine, though it would be nice to have a quick comment at the top of the class describing the reason for/the difference between nsRegion and nsIntRegion.
Attachment #411137 - Flags: review?(jmuizelaar) → review+

Updated

9 years ago
Duplicate of this bug: 527334
http://hg.mozilla.org/mozilla-central/rev/4f811113ea14
http://hg.mozilla.org/mozilla-central/rev/ce753b634dd1
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 192 landing]
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091110 Minefield/3.7a1pre ID:20091110030739

Robert, would it be possible to test with a reftest?
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.3a1
Status: RESOLVED → VERIFIED
No, reftests wouldn't catch this.
Ok, so this will still be covered in Litmus by https://litmus.mozilla.org/show_test.cgi?id=8349
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-litmus+
Is this bug actually in 1.9.2? I can't reproduce it there.
Jeff, the patch on bug 517802 hasn't been landed on 1.9.2. So it's trunk only.
Ah, OK. Sorry for that.
Flags: blocking1.9.2+
Whiteboard: [needs 192 landing]
You need to log in before you can comment on or make changes to this bug.