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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Dima Tisnek]

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:

window.open('http://www.freescale.com/files/graphic/product_freescale/AP13192USLK_IMG1.jpg','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

Comment 1

[Ria Klaassen (not reading all bugmail)]

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 browser.link.open_newwindow.restriction to 0 the image opens in a tab; in that case there is no problem.

Comment 2

[Ria Klaassen (not reading all bugmail)]

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 [e-mail]. Sorry if I bother the wrong person or if this is not a bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Ria Klaassen (not reading all bugmail)]

I discovered indeed a few builds without this bug.
Regression between 2004-09-09 08 and 2004-09-09 16.
Checkins: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2004-09-09+07%3A00&maxdate=2004-09-09+17%3A00

Comment 6

[Boris Zbarsky [:bzbarsky]]

Indeed.  Over to Chris.

Comment 7

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

After some testing I determined that this is not related to imagedocument.  Testcase in a moment.

Comment 8

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

Attachment: [part 2 of testcase]

Comment 9

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

Attachment: testcase

Testcase with no images

Comment 10

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

Attachment: even simpler testcase

Comment 11

[Boris Zbarsky [:bzbarsky]]

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").

Comment 12

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

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  Would have been nice if the testcases said that...

Comment 14

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

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

Comment 16

[Justin Dolske [:Dolske]]

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.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

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?

Comment 18

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

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.

Comment 19

[Justin Dolske [:Dolske]]

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 20

[Johnny Stenback (:jst)]

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.

Comment 21

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

Can you file a bug on the root cause / finding the root cause?  The testcase gets harder if you fix this bug ;)

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (plus test)

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.

Comment 23

[Justin Dolske [:Dolske]]

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

[Damon Sicore (:damons)]

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

Please re-request approval once reviews are completed.

Comment 25

[Damon Sicore (:damons)]

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

a1.9+=damons

Comment 26

[Justin Dolske [:Dolske]]

Checking in content/html/document/src/nsImageDocument.cpp;
  new revision: 1.175; previous revision: 1.174
Checking in content/html/document/test/Makefile.in;
  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

Comment 27

[Andrew Schultz]

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