[HiDPI] Add device pixel zoom level to image viewer

REOPENED
Assigned to

Status

()

Firefox
General
REOPENED
5 years ago
6 days ago

People

(Reporter: Greg Edwards, Assigned: jfkthame)

Tracking

(Blocks: 4 bugs, {regression})

22 Branch
Firefox 23
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
When running in a high DPI mode (Windows >96DPI with Bug 844064 landed, Retina Mac), the image viewer upscales the previewed image so that one pixel corresponds to one CSS pixel.

This leads to previewed images potentially looking very fuzzy, especially when using one of the non-int scaling factors on Windows. It would be useful to add a function to "zoom out" the image to device pixels. It would be ideal if by clicking the image, it would alternate between the three settings of Fit to Window, CSS Pixels, and Device Pixels.

Presently, you can zoom out using ctrl+mousewheel, etc., but device pixels is just another option that can be hard to tell apart, and, IIRC, isn't offered on some of the Windows scaling factors.

It was a tossup to me whether to file this as an enhancement or not, but I think given reactions to Bug 844064, it should be filed as a bug and considered a regression of that. We may even want to make device pixel zoom the default setting on Windows when it's eg. less than 1.5x.
(Reporter)

Updated

5 years ago
Blocks: 820679, 844604, 785330
(Reporter)

Comment 1

5 years ago
Sorry, I typed the wrong bug number. It should be Bug 844604 in the description.
Keywords: regression

Comment 2

5 years ago
When high-DPI is enabled, viewing certain images such as pixel art or a screenshot indeed looks blurry. Although this is intentional, I agree that it would be a better experience if images were scaled back down to 1x, as if the DPI was still 96.
(Assignee)

Comment 3

5 years ago
Did the same issue exist prior to bug 844604 for users who set a default full-page zoom factor using one of the various add-ons such as NoSquint? (Though in that case, I suppose it would still have been easier to manually reset the zoom when viewing an image.)
(Reporter)

Comment 4

5 years ago
I don't think so.
Duplicate of this bug: 788907
A few thoughts from bug 788907. We should do something, so large images make use of the extra hardware pixels. But I think smaller images should probably continue to be upscaled, since otherwise they can be awkwardly small.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 7

5 years ago
Defaulting to Fit to Window should deal with this already, right?

Another idea I had was to pick a zoom based on the image's embedded DPI, but unfortunately this setting is rarely used for its intended purpose and may carry a default value of 72 or 300 with little thought put into it.

A third option would be defaulting to device pixels if CSS pixels would be bigger than the window.

Certainly, as we've discussed here, device pixels should be the default zoom setting on 120DPI.

Re: your Bug 788907: Directly viewing Flash applets is providing the full resolution for me on both Win and OSX. (but see Bug 820831 on Windows) I think HiDPI support is up to the plugin author to provide. Firefox's native PDF viewer and its audio/video viewer are also HiDPI for me.
(Assignee)

Comment 8

5 years ago
(In reply to Greg Edwards from comment #7)
> Defaulting to Fit to Window should deal with this already, right?

The viewer's Fit to Window functionality only shrinks large images so that they fit within the window; AFAIK, it doesn't enlarge small ones (which would often look awful - imagine viewing a typical icon that's 32px or 64px square, and having Firefox expand it to fill a large window).
(Assignee)

Comment 9

5 years ago
Created attachment 738104 [details] [diff] [review]
add a device-pixel zoom level to image viewer

Here is a possible patch that implements a device-pixel setting, so that a large image will now cycle through three possible sizes when clicked (if device px != css px): the shrink-to-fit size, the device-pixel size, and the original css-pixel size. I have started a tryserver build (https://tbpl.mozilla.org/?tree=Try&rev=2955479e88ec); testing and feedback welcome once the build is available.
(Assignee)

Updated

5 years ago
Assignee: nobody → jfkthame
(Assignee)

Comment 10

5 years ago
Comment on attachment 738104 [details] [diff] [review]
add a device-pixel zoom level to image viewer

One thing I didn't do yet in this patch is to modify the behavior of the toggleImageSize method. AFAICS, that's not actually used anywhere in Firefox code, though of course it may be that addons or other applications make use of it. Should it have the same "cycling" behavior as introduced for the click event, or would it be better to leave it untouched and provide a new method for the new behavior?
Attachment #738104 - Flags: review?(bzbarsky)
(Reporter)

Comment 11

5 years ago
That depends entirely on how it's being used in the wild and if changing it would break anything.
toggleImageSize is used in one of our tests and in the SeaMonkey "Fit Image to Window" context menu option.

There's no obvious usage in addons.

That context menu option is a checkbox thing, so making it a tristate would be weird.  Probably better to just file a bug on SeaMonkey asking what they want here...
(Reporter)

Comment 13

5 years ago
Jonathan: It seems to be working correctly when I click the image to zoom, but when full zooming enters the mix, things get hairy. (I think the full zoom problems have been around all along, however.)

Ideally, both the click-zoom and full zoom should share the same underlying zoom state.

It correctly disables for me with devPixelsPerPx=1 and can tell when fit-to-window shouldn't be used.

I also think that device pixel zoom should be the default for 120DPI. Windows users often set this without expecting the size of images to change at all.
(Assignee)

Comment 14

5 years ago
The impression I had from the code was that using the click-to-zoom option was intended to cancel any full-zoom and return to switching the image between its "standard" sizes. But I haven't really tested that yet.

I see from the tryserver run that it's failing in content/html/document/test/test_bug369370.html, which relates to click-zoom behavior; I suspect this just means the test needs to be updated, but I'll need to look into it further to make sure.

(Note that the exact behavior of click-to-zoom -is- changed, even for the devPixelsPerPx=1 case, because the old code was not actually maintaining the image pixel at the clicked position very well. In my testing, the patch provides significantly better behavior, but this is probably why the existing test fails.)

> I also think that device pixel zoom should be the default for 120DPI. Windows users
> often set this without expecting the size of images to change at all.

I'd suggest that we handle this as a followup bug. I think it's debatable which should be the default. Yes, the device-pixel size avoids scaling and associated blurriness; but if we make it the default, then the (default) size of an image viewed "standalone" will be different from the default size of that same image within a web page (which -must- remain CSS-px based, so as not to disrupt layout). In general, changing the logical DPI setting is expected to change the size of -everything- on the screen: text, UI widgets, images, ....

So let's address that question separately. We can implement the additional scaling option as a first step, and then consider whether to change the default.
Comment on attachment 738104 [details] [diff] [review]
add a device-pixel zoom level to image viewer

>+  float GetCSSPixelToDevPixelRatio() {

In my head this is the ratio of the size of a CSS px to the size of a device px, whereas this function actually returns the reciprocal of that.

Maybe call this GetDevicePixelSizeInCSSPixels or something?

>+  float imageX = (aX + scroll.x) / ratio;

Why do we need to divide the scroll.x by ratio as well?  I have to admit I'm not following which coordinate aX is in and why we want to do things this way....

>+  nsCOMPtr<nsIDOMHTMLElement> htmlElement = do_QueryInterface(mImageContent);
>+  htmlElement->GetOffsetLeft(&left);
>+  htmlElement->GetOffsetTop(&top);

  nsGenericHTMLElement* htmlElement =
    nsGenericHTMLElement::FromContent(mImageContent);
  int32_t left = htmlElement->OffsetLeft();

Or just static_cast mImageContent to HTMLImageElement.

That said, do we really want the offsets rounded to integer CSS px?  Or should we be using something more like GetBoundingClientRect?

>+  nsCOMPtr<nsIDOMHTMLImageElement> image = do_QueryInterface(mImageContent);

Just static_cast mImageContent to HTMLImageElement.

>+      htmlElement->GetOffsetLeft(&left);
>+      htmlElement->GetOffsetTop(&top);

Don't you want clientLeft/Top here, if you want to compare it to event client coords, or better yet getBoundingClientRect?  Yes, I know you just moved this code.  :(

As things stand what this is doing is taking the viewport position of the event and subtracting the maybe-sorta-document position of the image's top-left corner... which is not necessarily a sensical thing to do.  That doesn't help the understandability of the code in ScrollImageTo.  Can we either get a lot better comments or just a clearer description of what the coordinate systems are?
(Assignee)

Comment 16

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #15)
> >+  float imageX = (aX + scroll.x) / ratio;
> 
> Why do we need to divide the scroll.x by ratio as well?  I have to admit I'm
> not following which coordinate aX is in and why we want to do things this
> way....

If (aX, aY) is a coordinate within the visible area of the (scaled) image - which might be scrolled so that its top-left is out of view - then we'd need to add the scroll value to this before "unscaling" it to original-image pixels in order to find the point of interest within the image.

But from the comments in nsIImageDocument.idl, I think this should be a coordinate within the complete image (at its current scale), so I've revised the patch to implement it this way.

(In the old code, click-to-zoom-in was only used when the image was currently at shrink-to-fit size, so there couldn't be any existing scroll offset to take into account; hence the question of whether the coords passed to these methods were relative to the entire image or the portion visible within the viewport didn't arise.)

> That said, do we really want the offsets rounded to integer CSS px?  Or
> should we be using something more like GetBoundingClientRect?

Given what this is used for, I don't think the precision really matters. But having said that, I've revised it to use GetBoundingClientRect, as suggested.

> As things stand what this is doing is taking the viewport position of the
> event and subtracting the maybe-sorta-document position of the image's
> top-left corner... which is not necessarily a sensical thing to do.

The image may not be occupying the full width or height of the viewport (unless aspect ratios happen to match exactly), so the event position in the viewport needs to be adjusted for the actual position within the viewport where the image is displayed. But this may not have been the best way to do that.

See if you find the revised patch more reasonable. Seems to work well locally. I still need to look into the click-to-resize test in content/html/document/test/test_bug369370.html that fails with the new behavior, though I think that's going to mean adjusting the test rather than the code.
(Assignee)

Comment 17

5 years ago
Created attachment 738469 [details] [diff] [review]
add a device-pixel zoom level to image viewer

Updated to make better sense, I hope.
Attachment #738469 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #738104 - Attachment is obsolete: true
Attachment #738104 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

5 years ago
Regarding test_bug369370.html: this is indeed affected by the (deliberately) changed zoom-in behavior.

In the old code, although the comments in nsIImageDocument.idl say that RestoreImageTo(x, y) will "try to keep a certain pixel in the same position", what it actually does is to try to center the given pixel (after scaling to the new size) in the viewport. Thus, a click near the top left of the original (reduced-to-fit) image will keep the scroll position at (0, 0), while a click near the bottom right will scroll to the maximum extent possible, so as to move the clicked pixel towards the center.

The new code actually implements what the original comment claimed: that click near (but not at) the top left will zoom in, as before, -and- adjust the scroll position such that the clicked position within the image stays exactly under the mouse. Likewise for the click near the bottom right; it will scroll the enlarged view just enough to keep the same point in the image under the mouse, rather than scrolling to the limit of the scrollable extent.

I think this is a better user experience; it also happens to match what the .idl file claimed we were doing all along! But test_bug369370.html was written assuming the old try-to-center-the-clicked-pixel behavior, and therefore will need adjustment.
(Assignee)

Comment 19

5 years ago
Created attachment 738475 [details] [diff] [review]
pt 2 - adjust click-to-resize-image test to account for revised zoom-in behavior
Attachment #738475 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

5 years ago
New tryserver build, including the test fix:
https://tbpl.mozilla.org/?tree=Try&rev=4fd56478ab70

To compare the old and new scroll-position behavior, try loading a large image such as http://upload.wikimedia.org/wikipedia/commons/d/d1/Mount_Everest_as_seen_from_Drukair2_PLW_edit.jpg in current Nightly. Choose a distinctive feature in the picture, such as one of the peaks, and click on it to enlarge to "full size". Notice how the chosen feature moves to (or towards) the center of the window, as far as the scrollable extent will allow, rather than staying under the mouse.

Then try the same with the tryserver build, and notice how the chosen feature of the image stays in place at the click position when the image zooms in. On a HiDPI display, this should work for both steps of the zoom, first to the device-pixel size, and then to the full CSS-pixel size.
(Assignee)

Updated

5 years ago
Blocks: 862803
(Assignee)

Comment 21

5 years ago
FTR, I've filed bug 862803 to bring this to attention for SeaMonkey.

Comment 22

5 years ago
> FTR, I've filed bug 862803 to bring this to attention for SeaMonkey.
Thanks! I've alerted Neil and others on this.
Comment on attachment 738469 [details] [diff] [review]
add a device-pixel zoom level to image viewer

>+  nsIDOMClientRect* clientRect;
>+  htmlElement->GetBoundingClientRect(&clientRect);

This is leaking the rect.

I recommend something more like this:

  nsRefPtr<nsClientRect> clientRect = htmlElement->GetBoundingClientRect();
  float origLeft = clientRect->Left();
  float origTop = clientRect->Top();

and similar for the after-resize bits.

>+  // Now update scroll to put the desired image pixel (at the new scale) at
>+  // its original position

I agree with the formula you're using here, but it took me a while to convince myself that it's the right thing...  I think maybe the right way to document it is that (clientLeft + imageX*ratio) is the new viewport-relative position of the click point, in CSS px, while (origLeft + aX) is the old viewport-relative position.  So we want to scroll by the difference between the two.  It may also be clearer to write it out as:

  scroll.x +=
    nsPresContext::CSSPixelstoAppUnits((clientLeft + imageX * ratio) -
                                       (origLeft + aX));

to make that more explicit.

>+      x -= NSToIntRound(left);

If you only want integer client coords anyway, just use htmlElement->ClientLeft() and ClientTop().  If you do want to keep doing GetBoundingClientRect, see above for the way to do it here.

r=me with those bits fixed.  Thanks for dealing with all the coordinate stuff!
Attachment #738469 - Flags: review?(bzbarsky) → review+
Comment on attachment 738475 [details] [diff] [review]
pt 2 - adjust click-to-resize-image test to account for revised zoom-in behavior

r=me; you presumably want to fold these together when landing....
Attachment #738475 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 25

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #23)
> Comment on attachment 738469 [details] [diff] [review]
> add a device-pixel zoom level to image viewer
> 
> >+  nsIDOMClientRect* clientRect;
> >+  htmlElement->GetBoundingClientRect(&clientRect);
> 
> This is leaking the rect.
> 
> I recommend something more like this:
> 
>   nsRefPtr<nsClientRect> clientRect = htmlElement->GetBoundingClientRect();
>   float origLeft = clientRect->Left();
>   float origTop = clientRect->Top();
> 
> and similar for the after-resize bits.

Now I'm confused.

When htmlElement is an HTMLImageElement*, the compiler won't let me call the zero-argument GetBoundingClientRect() on it:

 0:00.18 /usr/bin/make -C content -j8 -s
 0:01.09 ImageDocument.cpp
 0:02.34 /Users/jkew/mozdev/mc/content/html/document/src/ImageDocument.cpp:508:66: error: too few arguments to function call, expected 1, have 0
 0:02.34   nsRefPtr<nsClientRect> cr = htmlElement->GetBoundingClientRect();
 0:02.34                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
 0:02.34 ../../../../dist/include/mozilla/dom/HTMLImageElement.h:36:3: note: 'GetBoundingClientRect' declared here
 0:02.34   NS_FORWARD_NSIDOMELEMENT_TO_GENERIC
 0:02.34   ^
 0:02.34 ../../../../dist/include/mozilla/dom/Element.h:1519:79: note: expanded from macro 'NS_FORWARD_NSIDOMELEMENT_TO_GENERIC'
 0:02.34 }                                                                             \
 0:02.34                                                                               ^
 0:02.34 ../../../../dist/include/nscore.h:225:29: note: expanded from macro '\
 0:02.34 NS_IMETHOD'
 0:02.34 #define NS_IMETHOD          NS_IMETHOD_(nsresult)
 0:02.34                             ^
 0:02.34 ../../../../dist/include/nscore.h:173:27: note: expanded from macro 'NS_IMETHOD_'
 0:02.34 #define NS_IMETHOD_(type) virtual IMETHOD_VISIBILITY type
 0:02.34                           ^
 0:02.45 1 error generated.

But AFAICS, HTMLImageElement is derived (via a chain of classes) from mozilla::dom::Element, which provides that version of the method; so why the error?

If I static_cast it to nsGenericHTMLElement* instead of HTMLImageElement*, however, the compiler is happy.

> >+      x -= NSToIntRound(left);
> 
> If you only want integer client coords anyway, just use
> htmlElement->ClientLeft() and ClientTop().

Those aren't equivalent - they're derived from GetClientAreaRect, and don't seem to account for the current scroll offset (if any). So I'm staying with nsClientRect->Left() and Top().

(Rounding to integers here only because the existing RestoreImageTo(x, y) method, which is exposed via idl, is defined to take integer coordinates, and it doesn't seem worth the disruption of changing that to float.)
(Assignee)

Comment 26

5 years ago
Created attachment 738976 [details] [diff] [review]
add a device-pixel zoom level to image viewer.

Folded in the test fix, and updated patch as per review comments; carrying forward r=bz. Though I'd still be interested to understand why I ran into the error described in comment 25; I seem to be slightly lost among the layers of classes...
(Assignee)

Updated

5 years ago
Attachment #738469 - Attachment is obsolete: true
> so why the error?

Because C++ has these shadowing rules where a function with the same name but a different signature on a subclass will hide the one on the superclass, unless the subclass explicitly pulls it in via using or something.  :(  I guess we should just cast to nsGenericHTMLElement for now.  Or just use AsElement(), for that matter, to get is as an Element*, like so:

  nsRefPtr<nsClientRect> clientRect = mImageContent->AsElement()->GetBoundingClientRect();

> Those aren't equivalent - they're derived from GetClientAreaRect

Uh... so they are.  That's messed up; "client" coordinates are supposed to be viewport-relative.  Gotta love the web platform.  :(  And thank you for double-checking that!
(Assignee)

Comment 28

5 years ago
Created attachment 739086 [details] [diff] [review]
add a device-pixel zoom level to image viewer.

Thanks for the comments; updated to use AsElement(). Carrying over r=bz.
(Assignee)

Updated

5 years ago
Attachment #738976 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b85353b6cc38
Target Milestone: --- → Firefox 23
https://hg.mozilla.org/mozilla-central/rev/b85353b6cc38
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 863696
Uhh, this is giving me 3-state zoom on some images, and other images are changing zoom levels when they don't need it. This seems really weird, is it intentional?

I'm also surprised to see this suddenly implemented and landed without any kind of UX review.

I think we should probably back this out until have a solid idea of what the expected interactions are.
(Assignee)

Comment 32

5 years ago
(In reply to Justin Dolske [:Dolske] from comment #31)
> Uhh, this is giving me 3-state zoom on some images, and other images are
> changing zoom levels when they don't need it. This seems really weird, is it
> intentional?

3-state zoom on some images is intentional; that's exactly what was requested here. If you're on a hi-dpi system then "device-pixel size" and "full size" are different, and if it's a large image then there's also the shrink-to-fit size.

"Changing zoom levels when they don't need it" - I'd need a more specific description (with example) to be able to tell whether that's a bug or can be explained as expected behavior.

> 
> I'm also surprised to see this suddenly implemented and landed without any
> kind of UX review.
> 
> I think we should probably back this out until have a solid idea of what the
> expected interactions are.

The "expected interactions" were described in comment #0: "It would be ideal if by clicking the image, it would alternate between the three settings of Fit to Window, CSS Pixels, and Device Pixels."

But I'm fine with backing it out if there are concerns about the change in behavior; let's see if the UX folk have a view on this.

Comment 33

5 years ago
> But I'm fine with backing it out if there are concerns about the change in
> behavior; let's see if the UX folk have a view on this.

As a UX'er, I like it overall. There are a few regressions I noticed that I'll file shortly.

I wonder if flagging myself for needinfo works, so I can remember to elaborate in another comment tomorrow...
Flags: needinfo?(fyan)

Comment 34

5 years ago
Jonathan, I just want to preface this by saying that all your HiDPI work has been awesome and inspiring. I stood on your shoulders for all my HiDPI front-end work, and I thank you for that.

In particular, I really like this feature, but I think the execution needs some work and should be backed out of trunk, until the following points are addressed.

In its current landed state, this causes several regressions:
- The document title doesn't update properly when switching between zoom levels, i.e. it doesn't report the correct percentage.
- After switching zoom levels at least once, the image no longer fits to the window as the window is resized.

Regarding the overall UX, I discussed this with Limi today, and we agreed on the following:
The 3-state cycling for large images is a heavy tradeoff. It's less intuitive than binary zooming, and the feature is primarily useful for screenshots and developers. We think that the device pixel zoom level should be available as a non-persistent context menu item like "Play Speed" in our stand-alone video viewer. Since there are only two settings, the item can simply be a checkbox inline in the menu rather than a nested menu like "Play Speed".
The item should only be visible in windows with device pixel ratio greater than 1.
I'm rather awful at naming, so feel free to suggest a label for this context menu item.
Flags: needinfo?(fyan) → needinfo?(jfkthame)
(Assignee)

Comment 35

5 years ago
(In reply to Frank Yan (:fryn) from comment #34)

I'm a bit puzzled by the issues you describe, as I don't see either of them here:

> In its current landed state, this causes several regressions:
> - The document title doesn't update properly when switching between zoom
> levels, i.e. it doesn't report the correct percentage.

With current Nightly on my system (Retina Mac), the title (as shown in the window titlebar) *is* updated with the current zoom level when switching to device-pixel (i.e. 50%) or shrink-to-fit (variable) sizes, and no zoom level is shown at the full-size setting. Isn't that the expected behavior? Or is there something else that should be updating as well?

> - After switching zoom levels at least once, the image no longer fits to the
> window as the window is resized.

Again, with current Nightly, the behavior I see is as I'd expect: if the image has been zoomed to full-size or to device-pixel size, it remains unchanged when resizing the window; but if it has been zoomed back to shrink-to-fit, it again resizes dynamically (and the percentage in the title bar updates) when resizing the window. Am I missing something about the issue you're having?

> Regarding the overall UX, I discussed this with Limi today, and we agreed on
> the following:

OK - in that case I'll back this out for now, until I (or someone) has time to rework the UI side.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 36

5 years ago
Backed out from inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/321a93ac6141
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 37

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #35)
> (In reply to Frank Yan (:fryn) from comment #34)
> > - The document title doesn't update properly when switching between zoom
> > levels, i.e. it doesn't report the correct percentage.
> 
> With current Nightly on my system (Retina Mac), the title (as shown in the
> window titlebar) *is* updated with the current zoom level when switching to
> device-pixel (i.e. 50%) or shrink-to-fit (variable) sizes, and no zoom level
> is shown at the full-size setting. Isn't that the expected behavior? Or is
> there something else that should be updating as well?

Steps to reproduce:
1. Open an image in a tab with a viewport NOT large enough to fit the image so that it shrinks-to-fit at a scaling between 50% and 100%.
2. Click the image. The image upscales to 100%. The titlebar updates correctly.
3. Click the image again. The image scales downscales to 50% (device pixel zoom level). The titlebar updates correctly.
4. Click the image a third time. the image scales to the original shrink-to-fit scaling, but the titlebar doesn't update correctly. This is a regression.

> > - After switching zoom levels at least once, the image no longer fits to the
> > window as the window is resized.
> 
> Again, with current Nightly, the behavior I see is as I'd expect: if the
> image has been zoomed to full-size or to device-pixel size, it remains
> unchanged when resizing the window; but if it has been zoomed back to
> shrink-to-fit, it again resizes dynamically (and the percentage in the title
> bar updates) when resizing the window. Am I missing something about the
> issue you're having?

Steps to reproduce:
1. Open an image in a tab with a viewport large enough to fit the image without downscaling.
2. Click the image. The image downscales to 50% (device pixel zoom level).
3. Click the image again. The image scales back to 100%.
4. Resize the window so that the viewport is too narrow to fit the image. The image should shrink to fit, but it doesn't. This is a regression.

Let me know if you can't reproduce these.

Thanks for the quick response. I hope we can land this again in a better form soon.
(Assignee)

Comment 38

5 years ago
(In reply to Frank Yan (:fryn) from comment #37)
> Steps to reproduce:
> 1. Open an image in a tab with a viewport NOT large enough to fit the image
> so that it shrinks-to-fit at a scaling between 50% and 100%.

Ah - interesting, this -only- appears to be a problem if the shrink-to-fit size is -between- the device-pixel and 100% sizes. I was trying with an image where shrink-to-fit is -less- than 50%, and in that case the titlebar always updates correctly. 

Anyhow, thanks for the detailed STR on both issues; with that, these would've been straightforward to fix, I think. But in any case, the revised UX will supersede them.
(Reporter)

Comment 39

5 years ago
I would find needing to check an item in a context menu EVERY time I view an image very tedious and annoying. (Certainly a worse UX than Jonathan's.) If the preference to use device pixels is saved, it might be more convenient.

Another UX approach would be to have a whole gradation of different zoom levels, similar to Photoshop or other image editors. Device and CSS pixels could then just be conveniently included as options in this progression.

FWIW, Photoshop CS6 Retina zooms out to device pixels when you choose "actual pixels." The image viewers included with OSX and Windows also default to device pixels.
https://hg.mozilla.org/mozilla-central/rev/321a93ac6141

Comment 41

5 years ago
(In reply to Greg Edwards from comment #39)
> I would find needing to check an item in a context menu EVERY time I view an
> image very tedious and annoying. (Certainly a worse UX than Jonathan's.) If
> the preference to use device pixels is saved, it might be more convenient.

We considered having a persistent preference, but the UI would then have to show the mode that user is in, or there would be a high chance of mode errors, where the users forget what mode they're in and then try to [ctrl]+[+] to zoom in for all images after they get stuck in "device pixel zoom" mode. For integration into devtools, we might be able to do something smart for images with file paths containing "2x" or something, but that's beyond the scope of this bug.

> Another UX approach would be to have a whole gradation of different zoom
> levels, similar to Photoshop or other image editors. Device and CSS pixels
> could then just be conveniently included as options in this progression.

We'd love to have continuous zoom in the standalone image viewer, but the desktop versions of Firefox do not support continuous zoom yet.

> FWIW, Photoshop CS6 Retina zooms out to device pixels when you choose
> "actual pixels." The image viewers included with OSX and Windows also
> default to device pixels.

It's not especially useful to compare Firefox to photo editing and image viewing software. Firefox is a web browser and should show how an image would be displayed on the web by default (e.g. <img src="path.png"> with no hardcoded width/height).
(Assignee)

Comment 42

5 years ago
(In reply to Greg Edwards from comment #39)
> Another UX approach would be to have a whole gradation of different zoom
> levels, similar to Photoshop or other image editors. Device and CSS pixels
> could then just be conveniently included as options in this progression.

I thought about that - e.g., an image could cycle through a sequence of sizes such as 25%, 50%, 100%, 200%, with fit-to-window inserted into the sequence at an appropriate place -
but felt it would be a more substantial "new feature" and should probably be considered separately. It might be nice, for example, for small images to actually zoom to 200% when clicked, if that still fits within the window.

Comment 43

11 months ago
The problem still stands. Can we have the 3-state zoom included as an option in about:config, while the UX is still in question?
You need to log in before you can comment on or make changes to this bug.