207219 - Image Resize: Clicking to restore large size should center at clicked position

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Sebastian Lisken]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3) Gecko/20030312

If you view a large image in resized form (so it fits the canvas) and
then click to enlarge it to original size, it is always displayed from
scroll position (0, 0), if that expresses it correctly, in other words,
the upper left corner of the display corresponds to the upper left
corner of the image. If I have clicked the lower right corner of the
image I would expect that corner to be visible in the display, but it
might be a large scrolling distance away. I suggest that the pixel you
click on should always be displayed, e.g. as center of the display, or
or better it should not move, i.e. scroll the image so that the pixel
under the mouse cursor before clicking is also under the mouse cursor
after clicking. If possible, of course.

Reproducible: Always

Steps to Reproduce:
1. Open the image mentioned above (2048x2048 JPEG, 524 KB)
2. Make sure it's resized (the full image is visible)
3. Click near the lower right corner (i.e. on Australia)

Actual Results:  
The full-scale version appears, but only the upper left part (Europe) is visible.

Expected Results:  
Center on Australia. The clicked pixel should either appear in the center
of the canvas, or it should appear at the same canvas position where it
was before. Calculate a scroll position to achieve that as much as possible.

Comment 1

[Boris Zbarsky [:bzbarsky]]

.

Comment 2

[Jan Varga [:janv]]

good idea

Comment 3

[Dan Backes]

good idea indeed

Comment 4

[Kalin Kozhuharov]

Voted for!
It really drives me nuts sometimes.
But good that we have resizing anyway :-)

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

yes, I agree 100 %. I just hit the problem browsing a very large+high image.

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

a. get click point x and y coordinates in nsImageDocument::HandleEvent when
   the event is "click"
b. pass x and y to ToggleImageSize() if the event is "click"
c. pass 0 and 0 in all other occurences of ToggleImageSize() call
d. modify nsImageDocument::ToggleImageSize so it gets x and y coordinates
   pass x and y  to RestoreImage()
e. pass 0 and 0 in all other occurences of RestoreImage() call
f. in nsImageDocument::RestoreImage, compare x/y to current w/h and
   get final x/y in a restored image's coordinate system
g. scroll vertically and horizontally to ensure the x/y point is visible,
   preferably in the center of the viewport

Comment 7

[Sebastian Lisken]

Thanks a lot for that comment. Seems like the kind of comment we need
to move this forward. One objection to your last point.

> g. scroll vertically and horizontally to ensure the x/y point is visible,
>    preferably in the center of the viewport

When I came up with this idea, I wrote "The clicked pixel should either
appear in the center of the canvas, or it should appear at the same canvas
position where it was before". Having spent more time thinking about this
question, I still think that this second option is better. I think it's
what people expect naturally, and it causes less "movement" around the
clicked point, so I think people will have an easier time "finding their
way" around the new display of the picture. So I propose:

g. scroll vertically and horizontally to ensure the x/y point is visible,
   preferably in the original position of the viewport

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I strongly disagree with your proposal, and I think it will be EXTREMELY
confusing for a vast majority of non-geek users.

Comment 9

[Sebastian Lisken]

I'm surprised - especially about the amount of passion. Could you try to
make me understand your position? I think my arguments were not directed
at "geeks". And I think most programs that feature a "zoom" function behave
in this way. Could we do some research about that?

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Most of programs implementing a "zoom out" feature center the enlarged view
at the click location. I have at least 6 programs on the laptop in front of
me behaving that way, and just none behaving according to your proposal.

Comment 11

[Sebastian Lisken]

Yes, I have just learned that as well, testing a commercial program and
the GIMP. I was wrong. Based on that research, I agree that we should
center on the clicked pixel (if possible of course). I would still prefer
the other behaviour personally, but this is about Mozilla behaving in a
way natural to most users. Thanks for correcting me.

Comment 12

[Sebastian Lisken]

We're talking about "zoom in", not "out", by the way - I'm sure
that's what you did in your research, it's certainly what I did.

Comment 13

[[e-mail]]

Attachment: First go at a patch

Just looking for general comments at this stage.

Comment 14

[[e-mail]]

Attachment: Updated patch

I adjusted the zoom code slightly as per biesi's comments.

I also changed mImageElement to mImageComment, I didn't see the point of the QI
to an element just to set a few attributes.

Comment 15

[[e-mail]]

Comment on attachment 157420 [details] [diff] [review]
Updated patch

Oops, I meant mImageContent :-[

Comment 16

[Jan Varga [:janv]]

Comment on attachment 157420 [details] [diff] [review]
Updated patch

Looks good, anyway it seems it would be nicer to extend RestoreImage() and
ToggleImageSize() as Daniel suggested

Comment 17

[Jan Varga [:janv]]

I changed my mind, rename ZoomImage() to RestoreImageTo() and add it to the IDL.
r=varga

Comment 18

[[e-mail]]

Attachment: Addressed comments

Comment 19

[Jan Varga [:janv]]

Comment on attachment 157442 [details] [diff] [review]
Addressed comments

looks good

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 157442 [details] [diff] [review]
Addressed comments

>Index: src/nsImageDocument.cpp
>===================================================================

>+  view->ScrollTo((nscoord)(aX / (ratio * mImageWidth) * PR_MAX(0, scrolledSize.width - portRect.width) + 0.5),
>+                 (nscoord)(aY / (ratio * mImageHeight) * PR_MAX(0, scrolledSize.height - portRect.height) + 0.5),
>+                 NS_VMREFRESH_IMMEDIATE);
>+    return NS_OK;

2 space indentation.

>@@ -473,7 +516,26 @@ nsImageDocument::HandleEvent(nsIDOMEvent
>     CheckOverflowing();
>   }
>   else if (eventType.EqualsLiteral("click")) {
>-    ToggleImageSize();
>+    if (mImageResizingEnabled) {

Make that

   else if (eventType.EqualsLiteral("click") && mImageResizingEnabled) {

Comment 21

[[e-mail]]

Fix checked in.

I removed the if (mImageResizingEnabled) as it's never disabled at that point.

Comment 22

[Greg Campbell]

can this cool new feature make it into aviary for 1.0?

Comment 23

[Asa Dotzler [:asa]]

We're not going to hold the release for this. If the developer or reviewers 
believe that this belongs on the branch, they can request approval.

Comment 24

[Sebastian Lisken]

Sorry to come in so late, but I doubt whether the fix is correct. I assume the
intention is to center on the clicked pixel if possible (point g. in comment
#6). I was looking forward to a test of this (the first time I've reported a bug
and seen it fixed) with the "earth" image I gavein the description.
Unfortunately the area near Australia is dark and has few distinctive features,
but try clicking on the end of the cloud band that extends towards Australia,
more or less parallel to Java and probably ending on the East coast of
Australia. When I did, on 1024x768 screen, it ended up way off the center, about
half-way between the center and the lower right corner. It was easy to scroll
the image to bring the point to the center, the image is large enough. This is
using Mozilla 1.8a4 on Windows 2000.

I've reopened the bug, sorry if that's a mistake, but it seems appropriate to me.

Comment 25

[[e-mail]]

Ah, well, I implemented the comment #0 version because it was easier ;-)

Comment 26

[Sebastian Lisken]

You mean my first idea to "scroll the image so that the pixel
under the mouse cursor before clicking is also under the mouse cursor
after clicking."? Well, a) no, you didn't (as the test I suggested in comment
#24 immediately shows), and b) comment #6 is probably right in saying the other
behaviour is more intuitive even though my feelings are still different.
Whatever your opinion may be on b), this clearly needs some more work because of a).

Comment 27

[Prognathous]

Changing Summary and Status Whiteboard to make it clearer why this bug is still
open.

Prog.

Comment 28

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: zoom to the right place

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 177610 [details] [diff] [review]
zoom to the right place

Please explain this fix.

Comment 30

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

The location you click will be centered, unless it's too close to the edge, in
which case it will be as close to the center as possible.

Comment 31

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Updating summary to reflect patch and behavior that makes the most sense.

Minussing for blocking-seamonkey1.0a.

Comment 32

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

Comment on attachment 177610 [details] [diff] [review]
zoom to the right place

1) lose the printf
2) use NSToCoordRound instead of (nscoord)(... + 0.5)
3) parenthesize like this
(aX/ratio)*context->PixelsToTwips()

Comment 33

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: to check in

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

checked in by bz

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

As best as I can tell, this is FIXED according to the testcase in comment 0 with
build 2005-04-18-05 on Windows XP.

Comment 36

[Sebastian Lisken]

Sorry for not commenting earlier. I got an email asking me (as the reporter) to
verify the bug. I'm on a slow network connection so I can't easily download a
nightly to do this, but I will do in the next few days. Though it doesn't seem
to matter now. ;-) But I want to see this.

Comment 37

[Sebastian Lisken]

No, it still doesn't work for me. I used my example image, clicked on the
northern tip of Sri Lanka, and in the enlarged image the tip came up a
considerable distance east of the mouse cursor. About a fifth of a screen's
distance, and slightly offset towards north as well. What is going on? Is it a
problem that there is another Mozilla installation on the machine which I didn't
remove? I don't think so, as I did install into a new directory and used a fresh
profile, and I checked using a Windows utility (Process Explorer) that the
process had no files from that installation open (the new one did create its own
GRE directory for example). I used a Nightly for the Mozilla Suite, build
2005042106, under Windows 2000, on a 1024x768 screen. I'd like to reopen this
bug yet again.

Comment 38

[Stephen Donner [:stephend] Not actively reading bugmail]

Having the reporter satisfied is always best...

reopening per comment 37

Comment 39

[Sebastian Lisken]

Oh dear, I'm very sorry. Still confused between the behaviour that's just
natural to me (same canvas position, i.e. clicked pixel has the same position on
the screen before and after clicking) and the one I came to support after
investigating what other programs do (clicked pixel moves to the center of the
screen). The image of Sri Lanka did move as described earlier, but it moved to
the center of the screen so it's just file. Pity I can't delete my previous comment.

I confirm that this works for me, and I'm glad to say the bug doesn't need to be
reopened.

Comment 40

[Sebastian Lisken]

Thanks, but it was my mistake - closing again.

Comment 41

[Stephen Donner [:stephend] Not actively reading bugmail]

Sebastian, no problem at all.  Thanks for the confirmation that this is indeed
still fixed, and, more importantly, that I verified this correctly.

Comment 42

[James Ross]

*** Bug 299415 has been marked as a duplicate of this bug. ***

Comment 43

[Boris Zbarsky [:bzbarsky]]

This caused bug 369370.