Closed Bug 369370 Opened 13 years ago Closed 12 years ago

pop-up window image zoom-out leads to wrong (broken) display


(Core :: Layout, defect, minor)

Not set





(Reporter: dimaqq, Assigned: Dolske)




(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0

given url contains a link with following javascript attached to it:'','product_picture','menubar=no,width=500,height=500,resizable=yes');

any other large image would do instead of the one in javascript.

the JPEG file is large, some 3000x2000 pixels, so firefox show a magnifying glass control - if you click to zoom in and then zoom out, zoom out appears not to work - in fact the visible part of the page is not redrawn - pageup/pagedown or arrow key restores display to what it should be.

I suspect the following happens:
when I zoom in somewhere in the image (somewhere far from the upper left corner), firefox has to pretend the full-size image is positioned at negative coordinates to fit the selected portion of the image on the screen.
when I consequently zoom out, firefox forgets to reset those negative coordinates.

Note: viewing this image in a regular window (not created with javascript) works alright, only

Reproducible: Always

Steps to Reproduce:
1. Use supplid url or use javascript:<supplied code>
2. zoom in to the bottom or right edge-ish of the image
3. try to zoom out
Actual Results:  
image appears not to zoom out

Expected Results:  
image should zoom out
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070205 Minefield/3.0a2pre

I see it in every build I tried.
With to 0 the image opens in a tab; in that case there is no problem.
The point seems to be the pref dom.disable_window_open_feature.scrollbars. When I change that to true, the zoom feature works in the pop-up. Don't know if it should also work with the default setting.

CC-ing Sorry if I bother the wrong person or if this is not a bug.
Well, one way or another this should work.  ;)  Not having scrollbars shouldn't break the UI.

I'd start with ccing whoever has the blame for the image zoom in/out code, I guess.  No idea whether the bug is in their code or deeper in layout.
Flags: blocking-firefox3?
Component: General → Layout: Images
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → layout.images
Version: unspecified → Trunk
Is this a regression from 1.7 or 1.8.0.x?  If so, when did this break?  That would help figure out who should be fixing.

Could someone attach a testcase directly to the bug?

Have you looked at the blame for the image code?  It looks like Ctho has done a whole bunch of work on it in the last few years, so it this is a regression, chances are it's his....
Ever confirmed: true
Flags: blocking1.9?
Keywords: qawanted
I discovered indeed a few builds without this bug.
Regression between 2004-09-09 08 and 2004-09-09 16.
Blocks: 207219
OS: Linux → All
Hardware: PC → All
Indeed.  Over to Chris.
Assignee: nobody → cst
Flags: in-testsuite?
After some testing I determined that this is not related to imagedocument.  Testcase in a moment.
Assignee: cst → nobody
Component: Layout: Images → Layout
QA Contact: layout.images → layout
Attached file testcase
Testcase with no images
There's no way I see to "scroll down" in the "even simpler testcase" (nor to "Make sure you're not scrolled all the way left" in the "Testcase with no images").
(In reply to comment #11)
> There's no way I see to "scroll down" in the "even simpler testcase" (nor to
> "Make sure you're not scrolled all the way left" in the "Testcase with no
> images").

Well, in the "Testcase with no images", if you can't see the text, you're not scrolled all the way left.  In the "even simpler testcase", you can use arrow keys to scroll down after you make the popup window less tall.
Ah, I see.  Would have been nice if the testcases said that...
(In reply to comment #13)
> Ah, I see.  Would have been nice if the testcases said that...

I removed the <p> with that part when simplifying them.  Sorry.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Duplicate of this bug: 429250
So, looking at the testcase I have in bug 369370, the problem seems to be that the view is scrolled when zooming in, but when zooming out again nothing is scrolled. Looking at the document in Firebug shows that body.scrollLeft and body.scrollTop are both set to large values (roughly the width/height of the unscaled image). So, the scaled version is presumable there in the window, but we're scrolled outside it's bounds and thus nothing is shown.

It seems like the simple fix would be  to have nsImageDocument::ShrinkToFit() scroll back to (0,0).

There might be more to this, though... Shouldn't we be showing scrollbars when we get into the state of the content being smaller smaller than the window size, but the view is scrolled? Seems like I've seen this kind of thing in other apps/contexts... You scroll back to where you expect to be, and then the scrollbars disappear.

That would help in the non-imagedocument testcase attached to this bug, where arbitrary content resizes itself. I'm not sure if it's possible (or even correct?) for the browser to explicitly scroll things back into view, but it also seems sucky to require content to handle this case. At least scrollbars let the user figure out what happened and get back to where they want.
Attached patch Patch v.1 (obsolete) — Splinter Review
This scrolls the view back to (0,0) when zooming out. It's a little hackish in the way it reuses the existing RestoreImageTo() code, it might be cleaner to instead add a new "ScollToOrigin()" and cut'n'paste a minimized version of the RestoreImageTo() code there?
Attachment #316141 - Flags: review?(jst)
Are you not just hiding a real bug?  I would have thought that Gecko should automatically scroll to a legitimate position whenever a document changes size.
Maybe, that's what I was asking in comment 16. OTOH, this has apparently been broken for almost 4 years so, so I'm tempted to suggest taking a simple/harmless bandaid while continuing to wait for the root problem to be fixed. :)
Comment on attachment 316141 [details] [diff] [review]
Patch v.1

Yeah, unless someone steps up to deal with the real issue here I'm fine with this for now.
Attachment #316141 - Flags: review?(jst) → review+
Can you file a bug on the root cause / finding the root cause?  The testcase gets harder if you fix this bug ;)
jst, not sure if you want to review the mochitest for this or not. [I verified it fails on OS X and Linux w/o the patch.] The code change is the same as the reviewed patch.
Attachment #316141 - Attachment is obsolete: true
Attachment #316376 - Flags: review?(jst)
Attachment #316376 - Flags: approval1.9?
(In reply to comment #21)
> Can you file a bug on the root cause / finding the root cause?  The testcase
> gets harder if you fix this bug ;)

I filed bug 429621 for fixing the general problem.

Comment on attachment 316376 [details] [diff] [review]
Patch v.1 (plus test)

Please re-request approval once reviews are completed.
Attachment #316376 - Flags: approval1.9?
Attachment #316376 - Flags: review?(jst) → review+
Attachment #316376 - Flags: approval1.9?
Comment on attachment 316376 [details] [diff] [review]
Patch v.1 (plus test)

Attachment #316376 - Flags: approval1.9? → approval1.9+
Assignee: nobody → dolske
Checking in content/html/document/src/nsImageDocument.cpp;
  new revision: 1.175; previous revision: 1.174
Checking in content/html/document/test/;
  new revision: 1.21; previous revision: 1.20
Checking in content/html/document/test/bug369370-popup.png;
  initial revision: 1.1
Checking in content/html/document/test/test_bug369370.html;
  initial revision: 1.1
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: qawanted
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
FYI, the relevant pref here, browser.enable_automatic_image_resizing, is false by default.  Only camino and firefox enable it.  You might want to explicitly enable the pref for this mochitest or add it to the prefs in the mochitest profile (and I'll try to figure out why seamonkey doesn't override it :)).
You need to log in before you can comment on or make changes to this bug.