Bug 32801 – Don't shrink by default always when zoom in/out is enabled for standalone images

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Images

[Mario Sanchez Prada]

DESCRIPTION:

Currently the ShrinksStandaloneImagesToFit setting in WebCore::Settings controls two things at the same time:

  - Whether an standalone image should show up automatically resized to window size when loaded
  - Whether it's possible to zoom in (and out) the image by clicking on top of it

So, if you now set that setting to 'true' you'll get that all standalone images will appear shrunk to window size by default, and clicking on them will enlarge them. However if you set it to 'false' the image will appear in 1:1 ratio *and* clicking on top of it will get no effect (no zoom out as the handler won't be even installed).

This means so far it's impossible to render standalone images to the 1:1 ratio by default while still being able to click on top of them to zoom in/out the image, as using 'true' will shrink by default and using 'false' will not install the mouse click handler for that.

Check epiphany bug 599997 for more details about the motivation of this bug (https://bugzilla.gnome.org/show_bug.cgi?id=599997)

PROPOSAL:

Basically I'd see two ways to fix this:

  1. Keep the same meaning for ShrinksStandaloneImagesToFit values and just *add a new setting* to control the initial status of an image when shrinking is activated: initially shrink it or not.

  2. Change the meaning of ShrinksStandaloneImagesToFit setting so it would control *only* whether the image is initially shrank or not, having the handler for the zoom in/out thing always installed.


The main advantage of (1) is that it doesn't affect to the behaviour of already existing applications, just extends the possibilities with a new setting (but adding a new setting could be seen as a disavantage also).

The main advantage of (2) is that it would keep things more simple, although it would be needed to take the change into account in already existing apps, and it would make impossible to disable the zoom.

I have patches for both the two possibilities but I'm not sure which one is the most correct one bearing in mind all the platforms, so opinions? (I'll attach patches right away)

[Mario Sanchez Prada]

Created attachment 45291 [details]
Patch for proposal 1

(In reply to comment #0)
> [...]
>   1. Keep the same meaning for ShrinksStandaloneImagesToFit values and just
> *add a new setting* to control the initial status of an image when shrinking is
> activated: initially shrink it or not.

Attached patch would implement this proposal

[Mario Sanchez Prada]

Created attachment 45292 [details]
Patch for proposal 2

(In reply to comment #0)
> [...]
>   2. Change the meaning of ShrinksStandaloneImagesToFit setting so it would
> control *only* whether the image is initially shrank or not, having the handler
> for the zoom in/out thing always installed.

Attached patch would implement this

[Alexey Proskuryakov]

This patch has been overlooked, because it isn't marked for review. Please see <http://webkit.org/coding/contributing.html> for more detail.

[Mario Sanchez Prada]

(In reply to comment #3)
> This patch has been overlooked, because it isn't marked for review. Please see <http://webkit.org/coding/contributing.html> for more detail.

Sorry, I'll rebuild the patches against the last version of trunk and will add review flags for both of them this time, although obviously, only one of those would get in  in the best case :-)

[Mario Sanchez Prada]

Created attachment 58659 [details]
Patch for proposal 1 

Here you have a patch for the first proposal done against the latest version of trunk.

Personally, this is the option I like the most, but do not pay much attention to that, just let me know what you think both of the proposal and the implementation :-)

[Mario Sanchez Prada]

Created attachment 58662 [details]
Patch for proposal 2

...and here is the new patch for the second proposal

[Mario Sanchez Prada]

This has been stuck for quite a long time and I wonder whether I should do something else other than ping people or keep commenting.

Adding Xan Lopez to CC, as he's the maintainer of Epiphany browser, which depends on this bug to get https://bugzilla.gnome.org/show_bug.cgi?id=599997 fixed.

[Adam Barth]

Comment on attachment 58659 [details]
Patch for proposal 1 

There doesn't seem to be much interested in this patch.  R- for lack of tests.

[Adam Barth]

Comment on attachment 58662 [details]
Patch for proposal 2

No tests.

[Mario Sanchez Prada]

Thanks Adam for reviewing the patches.

The lack of tests is a shame, actually, but I wrote those patches almost one year ago when I barely knew what a Layout Test was, so I'm not surprised I didn't add any... :-)

Anyway, now I'm busy fixing those bugs related to accessibility in the GTK port so I don't think I'll work for a while on this bug (hope that's ok), unless I find some contiguous spare time at some point to focus on that, but that will be unlikely until december, I'm afraid :-(

Once again, thanks a lot for reviewing it. Now I have a clear lead to follow to get this fixed.

[Mario Sanchez Prada]

I think 4 years, together with the fact that this patch does not apply any more, is prudent enough to resolve this as a won't fix.