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

VERIFIED FIXED

Status

--
enhancement
VERIFIED FIXED
16 years ago
11 years ago

People

(Reporter: lisken, Assigned: csthomas)

Tracking

Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -
blocking-seamonkey1.0a -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.
.
Assignee: jdunn → blaker
Component: Image: GFX → XP Apps: GUI Features
QA Contact: tpreston → paw

Comment 2

16 years ago
good idea
Assignee: blaker → varga
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Priority: -- → P5
Target Milestone: --- → mozilla1.5beta

Comment 3

16 years ago
good idea indeed
Depends on: 190545

Comment 4

15 years ago
Voted for!
It really drives me nuts sometimes.
But good that we have resizing anyway :-)
yes, I agree 100 %. I just hit the problem browsing a very large+high image.
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
(Reporter)

Comment 7

15 years ago
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
I strongly disagree with your proposal, and I think it will be EXTREMELY
confusing for a vast majority of non-geek users.
(Reporter)

Comment 9

15 years ago
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?
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.
(Reporter)

Comment 11

15 years ago
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.
(Reporter)

Comment 12

15 years ago
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.
Posted patch First go at a patch (obsolete) — Splinter Review
Just looking for general comments at this stage.
Assignee: varga → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Posted patch Updated patch (obsolete) — Splinter Review
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.
Attachment #150485 - Attachment is obsolete: true
Comment on attachment 157420 [details] [diff] [review]
Updated patch

Oops, I meant mImageContent :-[
Attachment #157420 - Flags: superreview?(peterv)
Attachment #157420 - Flags: review?(varga)

Comment 16

15 years ago
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

15 years ago
I changed my mind, rename ZoomImage() to RestoreImageTo() and add it to the IDL.
r=varga
Posted patch Addressed comments (obsolete) — Splinter Review
Attachment #157420 - Attachment is obsolete: true

Updated

15 years ago
Attachment #157442 - Flags: superreview?(peterv)
Attachment #157442 - Flags: review?(varga)

Updated

15 years ago
Attachment #157420 - Flags: superreview?(peterv)
Attachment #157420 - Flags: review?(varga)

Comment 19

15 years ago
Comment on attachment 157442 [details] [diff] [review]
Addressed comments

looks good
Attachment #157442 - Flags: review?(varga) → review+
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) {
Attachment #157442 - Flags: superreview?(peterv) → superreview+
Fix checked in.

I removed the if (mImageResizingEnabled) as it's never disabled at that point.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 22

15 years ago
can this cool new feature make it into aviary for 1.0?
Flags: blocking-aviary1.0?
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.
Flags: blocking-aviary1.0? → blocking-aviary1.0-
(Reporter)

Comment 24

15 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ah, well, I implemented the comment #0 version because it was easier ;-)
(Reporter)

Comment 26

15 years ago
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).
Product: Core → Mozilla Application Suite

Comment 27

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

Prog.
Flags: blocking1.8b2?
Summary: Image Resize: Clicking to restore large size should center at clicked position → Image Resize: Clicking to restore large size should center at clicked position (image should scroll so that cursor would remain exactly above the same pixel)
Whiteboard: Checked in patch scrolls image, but is not accurate enough
Assignee: neil.parkwaycc.co.uk → cst
Status: REOPENED → NEW
Priority: P5 → --
Target Milestone: mozilla1.5beta → mozilla1.8beta2
Attachment #157442 - Attachment is obsolete: true
Attachment #177610 - Flags: superreview?(peterv)
Attachment #177610 - Flags: review?(jan)
Comment on attachment 177610 [details] [diff] [review]
zoom to the right place

Please explain this fix.
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.

Updated

14 years ago
Flags: blocking-seamonkey1.0a?
Blocks: 190545
No longer depends on: 190545
Attachment #177610 - Flags: superreview?(peterv) → superreview?(roc)
Updating summary to reflect patch and behavior that makes the most sense.

Minussing for blocking-seamonkey1.0a.
Status: NEW → ASSIGNED
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a-
Summary: Image Resize: Clicking to restore large size should center at clicked position (image should scroll so that cursor would remain exactly above the same pixel) → [cst: r?] Image Resize: Clicking to restore large size should center at clicked position
Summary: [cst: r?] Image Resize: Clicking to restore large size should center at clicked position → Image Resize: Clicking to restore large size should center at clicked position
Whiteboard: Checked in patch scrolls image, but is not accurate enough → [cst: r?] Checked in patch scrolls image, but is not accurate enough
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()
Attachment #177610 - Flags: superreview?(roc)
Attachment #177610 - Flags: superreview+
Attachment #177610 - Flags: review?(jan)
Attachment #177610 - Flags: review+
checked in by bz
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago14 years ago
QA Contact: pawyskoczka → stephend
Resolution: --- → FIXED
Whiteboard: [cst: r?] Checked in patch scrolls image, but is not accurate enough

Updated

14 years ago
Target Milestone: mozilla1.8beta2 → ---
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.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 36

14 years ago
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.
(Reporter)

Comment 37

14 years ago
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.
Having the reporter satisfied is always best...

reopening per comment 37
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 39

14 years ago
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.
(Reporter)

Comment 40

14 years ago
Thanks, but it was my mistake - closing again.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Sebastian, no problem at all.  Thanks for the confirmation that this is indeed
still fixed, and, more importantly, that I verified this correctly.
Status: RESOLVED → VERIFIED
Blocks: 272818

Comment 42

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

Updated

11 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.