Closed Bug 369370 Opened 13 years ago Closed 12 years ago
pop-up window image zoom-out leads to wrong (broken) display
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.
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 firstname.lastname@example.org. 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.
Component: General → Layout: Images
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....
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
OS: Linux → All
Hardware: PC → All
Indeed. Over to Chris.
Assignee: nobody → cst
Status: NEW → ASSIGNED
After some testing I determined that this is not related to imagedocument. Testcase in a moment.
Assignee: cst → nobody
Status: ASSIGNED → NEW
Component: Layout: Images → Layout
QA Contact: layout.images → layout
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-
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.
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.
(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.
Comment on attachment 316376 [details] [diff] [review] Patch v.1 (plus test) a1.9+=damons
Attachment #316376 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
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.