526882 - incorrect image repainting on zoom when scrollbar values change

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Works on Linux. I think this is bug in nsChildView::Scroll where we don't move the invalid region correctly when we scroll.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe it wasn't a regression from bug 517802 per se, but an incomplete fix there.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Definitely a widget bug, and I'm taking the liberty of marking it blocking, since it could affect all kinds of Mac scrolling.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 410687 [details] [diff] [review]
fix

Thanks for figuring this out!

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Fixes the bug I noticed in the above comment.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(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'.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: nsIntRegion

Indeed, I stuffed it up

Comment 13

[Markus Stange [:mstange]]

(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?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 15

[Markus Stange [:mstange]]

Comment on attachment 411116 [details] [diff] [review]
fix v2

I see.

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

The "!= nsnull" is unnecessary, no?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, but I think it's clearer.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/4f811113ea14
http://hg.mozilla.org/mozilla-central/rev/ce753b634dd1

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, reftests wouldn't catch this.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so this will still be covered in Litmus by https://litmus.mozilla.org/show_test.cgi?id=8349

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Is this bug actually in 1.9.2? I can't reproduce it there.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jeff, the patch on bug 517802 hasn't been landed on 1.9.2. So it's trunk only.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, OK. Sorry for that.