348276 - Remember the most recent "shrink to fit" state

Status: RESOLVED WONTFIX

(Firefox :: Settings UI, defect, P5)

Description

[Simon Bünzli]

UI for browser.enable_automatic_image_resizing has been removed from the reworked Options dialog. As I understood it, that pref should now be toggled automatically when the user enables/disables "shrink to fit" for a given image. This doesn't happen. Especially for users who happened to disable that pref, there's now no way to revert it without resorting to about:config.

Options:
* remember "shrink to fit" state whenever the user enables/disables it
* (re)add the original UI to the new Options dialog
* tell users to use an extension to change this behavior

Comment 1

[Simon Bünzli]

-> pkasting, as per http://forums.mozillazine.org/viewtopic.php?p=2424378#2424378

Comment 2

[Phil Ringnalda (:philor)]

Nominating, less because we can't ship without remembering and more because we can't ship without either remembering or putting back the pref we took out because a certain lead project engineer claimed we already did remember.

Comment 3

[Peter Kasting]

Man, I'm beginning to wonder if I was completely imagining those comments and no one actually said that :(

Nevertheless, this definitely seems like the right thing to do:
* When a large image is displayed, shrink or not based on pref value (just like we do now)
* However, always enable toggling the resize mode via clicking (instead of disabling when the pref is off)
* When the user toggles, toggle the pref value too

This will very slightly morph the meaning of the pref from "enable resizing" to "resize by default", but it will ensure that, on first run, everyone still sees exactly the size their current pref value dictates, and it should allow people to change the "default" intuitively without resorting to about:config, now that the pref is gone.

Should be an easy patch, I'll try and get it done today.  Setting target milestone appropriately.

Comment 4

[Phil Ringnalda (:philor)]

(In reply to comment #3)
> Man, I'm beginning to wonder if I was completely imagining those comments and
> no one actually said that :(

bug 340677 comment 68

Comment 5

[Peter Kasting]

(In reply to comment #4)
> (In reply to comment #3)
> > Man, I'm beginning to wonder if I was completely imagining those comments and
> > no one actually said that :(
> 
> bug 340677 comment 68

Oh good, I'm not going crazy, I just misremembered who said it :)

Well, that should make this a slam-dunk for ui-r? then, I'll just send it to mconnor :)

Comment 6

[Peter Kasting]

OK, after discussion with mconnor on IRC< several things are true here:

On trunk, bug 197263 should have made the "enable resizing" pref remember last state when toggled off.

So what we think needs to be done is:
* Land that on branch
* Set the prefs such that "remember last state" is the default behavior
* Figure out how to translate users' existing prefs so that people still get the behavior they expect

I don't know how to accomplish that third point because I don't understand how our prefs work well enough.

Comment 7

[Peter Kasting]

Committed the fix for bug 197263 on branch.

Comment 8

[Phil Ringnalda (:philor)]

(In reply to comment #6)
> On trunk, bug 197263 should have made the "enable resizing" pref remember last
> state when toggled off.

I don't see any sign of pref-setting in the patch, and if I open a large image in today's trunk, click to show it unresized, then open another, the next one is still resized.

Comment 9

[Peter Kasting]

(In reply to comment #8)
> (In reply to comment #6)
> > On trunk, bug 197263 should have made the "enable resizing" pref remember last
> > state when toggled off.
> 
> I don't see any sign of pref-setting in the patch, and if I open a large image
> in today's trunk, click to show it unresized, then open another, the next one
> is still resized.

Yes, you're about 30 seconds ahead of me.  I've just been testing this, and it doesn't "remember" anything, it just makes toggling available when the pref is off, as far as I can tell.  I need to dig through the code more to figure out what's going on, why, and how.

Comment 10

[Peter Kasting]

OK, I think what I need here is two prefs instead of the current one (suggestions for better names welcome):

browser.automatic_image_resizing
  0 = Initially show images 100%
  1 = Initially show images shrink-to-fit
  other = Initially show images in last state [default]
browser.image_resized
  false = Image was at 100%
  true = Image was shrunk-to-fit [default]

We always write the second pref, but we only read it if the first pref is not 0 or 1.

I'm not sure whether users who changed browser.enable_automatic_image_resizing to "false" should get browser.automatic_image_resizing = 0 or browser.image_resized = false as the replacement.

Comment 11

[Peter Kasting]

(In reply to comment #10)
> I'm not sure whether users who changed browser.enable_automatic_image_resizing
> to "false" should get browser.automatic_image_resizing = 0 or
> browser.image_resized = false as the replacement.

They should definitely get browser.image_resized = false regardless.  I think they should also get browser.automatic_image_resizing = 0, because people who disabled this presumably want to show images at 100% by default "except for those times when they don't".

For my own notes, the way to migrate the old prefs is:
* Remove the old pref from the lists of prefs so it will not appear by default or have a default value
* In the pref-reading code in my patch, see if the new pref is in its default state.  If so, see if the old pref is readable (i.e. has a user-defined value saved).  If so, write the corresponding new values to the new prefs.
This will preserve everybody's behavior as much as possible, and allow users of new builds to go back to old builds nondestructively.

Comment 12

[[e-mail]]

An alternative option that you might consider would be a browser.remember_automatic_image_resizing pref that, when set to true, would make click-to-resize update the browser.enable_automatic_image_resizing pref.

Comment 13

[Peter Kasting]

(In reply to comment #12)
> An alternative option that you might consider would be a
> browser.remember_automatic_image_resizing pref that, when set to true, would
> make click-to-resize update the browser.enable_automatic_image_resizing pref.

That's basically identical to my design, except that it tristates the "remember" pref to give a settable default for the non-remember state.  I suppose it could be a boolean instead, though, and, when off, mean that we always start in the state dictated by the "enable" pref.

Maybe that's worth doing because of the simplicity in transitioning old prefs to new: we simply add a new pref and default it on and call it "good enough".

Comment 14

[Mike Connor [:mconnor]]

Not going to block on this at this point.  Going to follow up with beltzner to discuss the implications here, this might be best fixed in another way on trunk.

Comment 15

[Peter Kasting]

(In reply to comment #14)
> Not going to block on this at this point.  Going to follow up with beltzner to
> discuss the implications here, this might be best fixed in another way on
> trunk.

You might want to give me a minute.  I have a patch "done" (not yet finished compiling, and then need to test) that basically implements Neil's suggestion.  Once I know it works I'll post it here.  It's pretty low risk: it adds one additional pref and a pair of SetBoolPref() calls in the image shrink/restore functions.

Comment 16

[Peter Kasting]

Attachment: trunk patch v1

This patch adds a pref to control whether we remember the image resizing state.  It defaults to true.  There is no migration code needed for old prefs: the old image resizing pref remains as the "default state" for how all images come up.

The net effect is that the browser will first show a large image in whatever mode the "enable" pref says, you can always toggle modes (thanks to the fix for bug 197263), and by default, the browser remembers your toggled mode and uses it for the next large image.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #16)
> bug 197263), and by default, the browser remembers your toggled mode and uses
> it for the next large image.

That seems incredibly incorrect to me. Why would we assume that because someone wanted an image on one site to be expanded that they'd want *all* images to behave that way? Should we also remember their scroll position?

(IIRC, the resize to fit preference was removed due to comments in the dev.apps.firefox thread on the preferences re-org where Hixie mentioned that now that we had a toggle in the UI, it seemed less useful to expose the pref. I agree with that; by default, we should be avoiding shrink behaviour, and if the user wants the full image, they can click on it.)

Comment 18

[Peter Kasting]

(In reply to comment #17)
> (In reply to comment #16)
> That seems incredibly incorrect to me. Why would we assume that because someone
> wanted an image on one site to be expanded that they'd want *all* images to
> behave that way?

Because it's common to look at a series of large images and want them all the same way?  e. g. I go to some site with lots of big album covers, or a real estate site, and want to see images in full detail.  I toggle once and the browser remembers that's what I like and uses that for all my subsequent images, instead of forcing me to click on each one.

Much more importantly, this helps people who don't really want to frequently toggle between modes, but want a different default than we provide, and don't know how to get to about:config.  They click once to change things and the browser does the right thing ever after.

> Should we also remember their scroll position?

I'm not sure I understand the point you're trying to make here... we certainly do this for case sensitivity in the find bar, though!  :)

> (IIRC, the resize to fit preference was removed due to comments in the
> dev.apps.firefox thread on the preferences re-org where Hixie mentioned that
> now that we had a toggle in the UI, it seemed less useful to expose the pref.

Well, more specifically, we have the ability as of today to toggle in both modes.  And without this patch, we still force people to toggle every image if they don't want our default but don't use about:config.  That seems like a nonzero-size group.
 
> by default, we should be avoiding shrink behaviour, and if the
> user wants the full image, they can click on it.)

That sounded like you just said two opposite things?

Comment 19

[Phil Ringnalda (:philor)]

(In reply to comment #17)
> by default, we should be avoiding shrink behaviour, and if the
> user wants the full image, they can click on it.)

That would be a change from the current situation. I'm not going to thrash the flag, but if I hadn't thought earlier that we should block on some resolution here, the fact that we now have two reasons why we removed the pref, neither one of which are the case, would make me think so now.

Comment 20

[Peter Kasting]

> (In reply to comment #17)
> > by default, we should be avoiding shrink behaviour, and if the
> > user wants the full image, they can click on it.)
> 
> That would be a change from the current situation.

Oh, maybe comment #17 meant "we should default to showing all images at 100% instead of shrink-to-fit"?

If that's what it meant, then yes, we'd need to reverse the enable_automatic_image_resizing pref default from true to false to make that the case.

And if THAT happens, then I'd feel even more strongly about getting this change in, because there are a large class of people who like auto-resizing and DON'T want the default zoom to be 100%, and this patch is the easiest UI for them to get at what they want.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Actually it was just a typo. I meant that by default, we should be shrinking images to fit in the screen. 

If we think enough users encounter scenarios as you describe, we should either add the pref back in or change it to a "remember my state" pref. By default, remembering state for something like this is wrong and will just end up confusing users. We're doing the right thing by default now, let's not change that and instead revisit the choice to remove the pref. 

Comment 22

[Peter Kasting]

(In reply to comment #21)
> If we think enough users encounter scenarios as you describe, we should either
> add the pref back in or change it to a "remember my state" pref. By default,
> remembering state for something like this is wrong and will just end up
> confusing users. We're doing the right thing by default now, let's not change
> that and instead revisit the choice to remove the pref.

OK... then yes, I think we should certainly revisit that choice.  My IRC conversation with mconnor this morning led me to believe that he thought remembering was not only the right thing to do, but that we were already doing it, and that that was why it was OK to remove the pref; that's what I thought as well.

If we do re-add prefs, then I would re-add as "Resize large images to fit on my screen" or something like that, and have it toggle the value of the existing pref.  I think adding a user-visible pref for the "remember" pref is overkill even if the feature does go in.  If we re-add that pref, then I'm not as worried about this going in on branch (though it might be nice, defaulted to off, on trunk?).

FWIW I also got several comments this morning from people here who thought remembering seemed like the right thing to do, so I wonder whether it's as confusing as you believe.  It's hard for me to know; the auto-resize feature itself is inherently a bit confusing.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233297 [details] [diff] [review]
trunk patch v1

sr=me on the code.  For what it's worth, this does seem like a good idea to me... but then again, so does Emacs.  ;)

Comment 24

[Phil Ringnalda (:philor)]

Well, now that we're going to ship 2.0 with no visible UI for anything, and no chance to get this in, how about splitting your patch in two, the fully reviewed and sr-ed core part and the one line to set the pref in Firefox, so you can land the backend and those of us who can flip a pref can see how it works out?

Comment 25

[Peter Kasting]

(In reply to comment #24)
> Well, now that we're going to ship 2.0 with no visible UI for anything, and no
> chance to get this in, how about splitting your patch in two, the fully
> reviewed and sr-ed core part and the one line to set the pref in Firefox, so
> you can land the backend and those of us who can flip a pref can see how it
> works out?

I'm reluctant to check in additional functionality that's only under a default-off pref when it has gotten an implicit ui-r minus from beltzner.

Since we're only worried about trunk at this point, the right thing is probably to figure out (with beltzner) how to address comment 21.

Comment 26

[Simon Bünzli]

Beltzner: It might be worth to mention in the release notes how to revert the prefs which have become UI-less.

Comment 27

[Peter Kasting]

Taking myself off as the assignee; my thoughts and patch are above if anyone wants to push for something to come of this.

Comment 28

[Phil Ringnalda (:philor)]

So, what does it actually mean that this bug, which is about doing a specific thing, remembering the most recent shrink to fit state so that you can toggle your pref at any time by toggling any image, a thing for which it already has a properly reviewed and superreviewed patch that nobody's willing to check in because Firefox doesn't want it, is blocking-firefox3+?

Did you mean to set this bug to WONTFIX, and mark bug 390369 as blocking-fx3, or did you come around to seeing my usage pattern, going from a gallery where I want to see detail to a gallery where I don't, as more common (and harder to do without this, since it involves opening Preferences if we have UI, or about:config if we don't) than occasionally wanting to see single images in detail (which, with this, would just involve either toggling that image back down, or toggling the next unshrunk image down), and you're just waiting for someone to check in the patch?

Comment 29

[Simon Bünzli]

Beltzner: Any reaction to comment #28? I'd be fine with WONTFIXing this bug for good, considering that there are so few people asking for this (and FWIW I now agree with your comment #21).

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 233297 [details] [diff] [review]
trunk patch v1

I don't think this behaviour is clearly right, and since it's potentially confusing, going to minus for now.

Comment 31

[:Gijs (he/him)]

Given that we've now lived with this for 8 years, it seems like there is no consensus that changing the behaviour here would make sense, which then implies that this has practically been wontfix for 8 years. As I'm trying to clean up this particular component, I will go and actually change the bug status. If people feel very strongly that this still needs to change, I would suggest they file a new bug and ping the UX folks on it who currently make decisions about this stuff. It's not like the patch here would apply today anyway...