389756 - full page zoom interacts poorly with image auto zoom

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[(not currently active) Ted Mielczarek]

Using full page zoom when viewing an image seems to confuse image auto zoom.  It seems like the canvas is expanding in size, but the auto-zoom code keeps trying to shrink the whole thing to fit.

Comment 1

[Eli Friedman]

(In reply to comment #0)
> Using full page zoom when viewing an image seems to confuse image auto zoom. 
> It seems like the canvas is expanding in size, but the auto-zoom code keeps
> trying to shrink the whole thing to fit.

Yeah, that seems like a good description of the problem; the issue is that the image auto zoom code doesn't know what's going on.  I guess the image document code could query for the current zoom level and adjust as appropriate.

I'm not completely sure how exactly it should adjust, though; there doesn't seem to be an intuitive way to deal with the interaction between the scaling and the zoom/restore feature.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Can we make the image document understand full page zoom, and have it modify full page zoom to do the actual zoom?  That is, it should set the full page zoom to 1.0 or 0.x or whatever to zoom higher or lower; if the user then uses control-+/- to zoom, they'll just be modifying the same zoom variable.  Clicking the image will reset it to either shrink-to-fit or to 1:1.  How's that sound?

Comment 4

[Mike Schroepfer]

+ing since this is new with the fp zoom.  

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

I think this is the behavior I'd like to have.
+/- or clicking has the same effect as now, but scaled using zoom-in/-out level.
If there is some zooming used, resizing window doesn't affect to image size.
Comments?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> I'm not completely sure how exactly it should adjust, though; there doesn't
> seem to be an intuitive way to deal with the interaction between the scaling
> and the zoom/restore feature.
I agree with Eli, zoom/restore/resize and page scaling don't fit together
very well, but I do see use cases for them both.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

->DOM, since this is implemented in ImageDocument

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

This has behavior closer to what vlad described.
page-scale zooms in/out, mouseclick or +/- keypress restores the old style
shrink-to-fit/1:1.

Not sure who should review this, but since jst reviewed Bug 73322 (only 5 years ago ;) )...

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 301556 [details] [diff] [review]
v2

Or maybe still not good enough

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 301556 [details] [diff] [review]
v2

Bah, defining how this should work with resize-shrink-to-fit is hard, so perhaps the patch is enough, at least for now.

Comment 13

[Elmar Ludwig]

How will this behave when a global page zoom value is set (see bug 414636)?

This is probably not terribly important, since there is no UI in Firefox to set the global zoom (at least for now), but I'm just curious.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Have to test, but I guess I could test if there is global zoom value
and use that instead of 1.0

And if someone has good suggestion how this should work, comments welcome.
Comment 3 doesn't take account resize-shrink-to-fit behavior.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

If I read the code right, the global zoom level uses 
nsIContentPrefService which is under toolkit/.
I *really* don't want to make content/ depend on toolkit/.
Not all gecko apps are toolkit apps.
So unless that pref is changed to be set as a normal pref, there isn't much
ImageDocument can do.

And anyway, I think this feature doesn't even need to care about global 
zoom: either use 1:1/shrink-to-fit or whatever page zoom level there is.
Doesn't matter if page zoom level is set based on global zoom.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 301556 [details] [diff] [review]
v2

There's already been talk about moving the interface nsIContentPrefService into content so that we could use it from there, so if there's a need to do that now or later, we should just do that (but leave the implementation where it is and use the interface conditionally).

Either way, this seems like a step in the right direction, so r+sr=jst

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I dared to add SetZoomLevel(1.0) still to toggleImageSize (only for SM) and
also to OnStartContainer to make initial imagesize reasonable when zooming and overflowing.
It is not clear what user expects in different cases, so trying something, which
at least works comparing to current broken behavior :(