812529 - Work - Update selection monocle image

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Theme, defect, P1)

Description

[Jim Mathies [:jimm]]

Attachment: current image

We might want to update this. I created it using an 'o' in a gothic font. It's not perfectly round, but is about the same size as the default monocle the metro interface uses.

Comment 1

[Stephen Horlander [:shorlander]]

Attachment: Selection Monocle

The current size is an unusual 32x34, not sure if that is intentional. This is 18 x 18.

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: Selection Monocle - 140%

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Selection Monocle - 180%

Comment 4

[Jim Mathies [:jimm]]

hey fryn, curious if you could give me a pointer or two on adding 140%/180% image resources. I'm not sure how those integrate into our css.

Comment 5

[Frank Yan (:fryn)]

(In reply to Jim Mathies [:jimm] from comment #4)
> hey fryn, curious if you could give me a pointer or two on adding 140%/180%
> image resources. I'm not sure how those integrate into our css.

To follow a pattern similar to that which I used for 200% images on OS X, I think we could do the following in browser.css:

@media (min-resolution: 135dpi) {
    /* Load 140% image when scaled by 140% */
    :-moz-any(#selectionhandle-mark1, #selectionhandle-mark2, #selectionhandle-mark3) > .toolbarbutton-icon {
        list-style-image: url("chrome://browser/skin/images/[e-mail]");
        width: 32px;
        height: 24px;
    }
}

@media (min-resolution: 174dpi) {
    /* Load 180% image when scaled by 180% */
    :-moz-any(#selectionhandle-mark1, #selectionhandle-mark2, #selectionhandle-mark3) > .toolbarbutton-icon {
        list-style-image: url("chrome://browser/skin/images/[e-mail]");
    }
}

Resource: http://msdn.microsoft.com/en-us/library/windows/apps/hh465362.aspx

Comment 6

[Frank Yan (:fryn)]

Correction (I checked the wrong image for dimensions previously):

@media (min-resolution: 135dpi) {
    /* Load 140% image when scaled by 140% */
    :-moz-any(#selectionhandle-mark1, #selectionhandle-mark2, #selectionhandle-mark3) > .toolbarbutton-icon {
        list-style-image: url("chrome://browser/skin/images/[e-mail]");
        width: 18px;
        height: 18px;
    }
}

@media (min-resolution: 174dpi) {
    /* Load 180% image when scaled by 180% */
    :-moz-any(#selectionhandle-mark1, #selectionhandle-mark2, #selectionhandle-mark3) > .toolbarbutton-icon {
        list-style-image: url("chrome://browser/skin/images/[e-mail]");
    }
}

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch

This isn't worked exactly how I would expect it too, but maybe my assumptions are off. On the surface pro, the system dpi is 135. However if I use min-resolution:134dpi, the middle image doesn't get loaded. This might be a bug down in layout or widget, not sure. If I use 130dpi, it displays.

Also, despite having a width/height of 18px, the image is displayed at its real dims, so for example wit the middle image, an image of 25x25px is displayed. The result is a monocle image that isn't positioned exactly right.

Also, down in widget the actual dpi value returned by the system is 134.699 rather than 135. I'm wondering if we should be using 134dpi in css, or maybe we should round up the dpi we return from the system.

Comment 8

[Frank Yan (:fryn)]

(In reply to Jim Mathies [:jimm] from comment #7)
> This isn't worked exactly how I would expect it too, but maybe my
> assumptions are off. On the surface pro, the system dpi is 135. However if I
> use min-resolution:134dpi, the middle image doesn't get loaded. This might
> be a bug down in layout or widget, not sure. If I use 130dpi, it displays.

Strange. I don't know why that might be.

> Also, despite having a width/height of 18px, the image is displayed at its
> real dims, so for example wit the middle image, an image of 25x25px is
> displayed. The result is a monocle image that isn't positioned exactly right.

I'll look into this. It's probably a problem with the CSS, which is odd since this kind of CSS worked for OS X.

> Also, down in widget the actual dpi value returned by the system is 134.699
> rather than 135. I'm wondering if we should be using 134dpi in css, or maybe
> we should round up the dpi we return from the system.

I think we should quantize the dpi values we return to integer dpi, solely because there is a lot of code on the web that looks like:
@media (max-width: 479px) { ... }
@media (min-width: 480px) { ... }

and based on Microsoft's recommendation — http://msdn.microsoft.com/en-us/library/windows/apps/hh465362.aspx :
@media (max-resolution: 134dpi) { ... }
@media (min-resolution: 135dpi) { ... }

I bet there will be a lot more code that breaks if we return non-integer dpi.
Note that 1in (1 CSS inch) is always exactly 96px (96 CSS pixels), so with the quantization to integer dpi (dots per inch), we'll still have fractional dppx (dots per pixel).

Comment 9

[Jim Mathies [:jimm]]

Attachment: widget dpi fix v.1

Pushed this to try with some logging to get a reading on a non-highdpi system. On the Surface Pro widget is now returning 135.

Comment 10

[Jim Mathies [:jimm]]

Attachment: images and css

Comment 11

[Jim Mathies [:jimm]]

Attachment: widget dpi fix v.1

Looks ok, reports 96 on our test slaves, 135 on the surface pro. I also added some caching.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 763560 [details] [diff] [review]
widget dpi fix v.1

Review of attachment 763560 [details] [diff] [review]:
-----------------------------------------------------------------

FWICT, this works as expected.

r+ as long as the threading issue is addressed.

::: widget/windows/winrt/MetroUtils.cpp
@@ +43,5 @@
>  #endif
> +  ComPtr<IDisplayPropertiesStatics> sDispProps;
> +
> +  FLOAT LogicalDpi() {
> +    if (!sDispProps) {

This function isn't thread-safe. If multiple threads call it and make it past this check before sDispProps has been set, they will all call GetActivationFactory and thrash sDispProps. If it's possible that multiple threads will call this function before sDispProps is set, then we have to modify this to be thread-safe. If we're sure that that can't happen, then let's at least add a comment mentioning the threading issue. That way, if someone modifies the calling pattern of this function in the future, they'll know to be wary.

Comment 14

[Jim Mathies [:jimm]]

We're still getting lower dpi values for min-resolution than expected. I did a little debugging of the media query code - 

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsMediaFeatures.cpp#245

GetIsViewportOverridden returns false, so we end up using AppUnitsPerCSSInch here. The values are:

AppUnitsPerPhysicalInch: 5805
AppUnitsPerCSSInch: 5760
AppUnitsPerDevPixel: 43 

Need to figure out why these two slightly different. If we were using AppUnitsPerPhysicalInch our dpi would be correct, but the AppUnitsPerCSSInch value throws things off, resulting in a dpi of 134.95.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #14)
> We're still getting lower dpi values for min-resolution than expected. I did
> a little debugging of the media query code - 
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsMediaFeatures.
> cpp#245
> 
> GetIsViewportOverridden returns false, so we end up using AppUnitsPerCSSInch
> here. The values are:
> 
> AppUnitsPerPhysicalInch: 5805
> AppUnitsPerCSSInch: 5760
> AppUnitsPerDevPixel: 43 
> 
> Need to figure out why these two slightly different. If we were using
> AppUnitsPerPhysicalInch our dpi would be correct, but the AppUnitsPerCSSInch
> value throws things off, resulting in a dpi of 134.95.

actually - 133.953491.

Comment 16

[Jim Mathies [:jimm]]

Looks like this will get sorted out when we turn on apzc. There doesn't appear to be a way to get an accurate dpi value in media queries using the values we have. With GetIsViewportOverridden returning false, we end up dividing two integer values: appUnitsPerInch, which is hardcoded to 5760 (96*60), the the number of app units per device pixel (43). The result is a dpi value of ~133. 

Once GetIsViewportOverridden returns true with apzc turned on, we'll be working with AppUnitsPerPhysicalInch (5805) which would yield a 135 dpi.

Comment 17

[Jim Mathies [:jimm]]

Kicking this over to the hidpi assets story so I can close out chrome selection work.

Comment 18

[Jim Mathies [:jimm]]

Attachment: css

Comment 19

[Jonathan Wilde [:jwilde]]

Comment on attachment 789721 [details] [diff] [review]
css

Review of attachment 789721 [details] [diff] [review]:
-----------------------------------------------------------------

r=jwilde with a few tweaks.

::: browser/metro/theme/browser.css
@@ +290,5 @@
>  .selection-overlay-hidden {
>    display: none;
>  }
>  
>  .selectionhandle {

I think we need to set an explicit width/height on .selectionhandle. See .appbar-primary (and, more specifically, .appbar-primary > .toolbarbutton-icon) styles for reference.

Right now, it looks like the size of the image increases with the screen pixel width/height associated with the image, compounded by the device-screen pixel scaling factor. This makes the handles a little larger than we want, I think. :-)

::: browser/metro/theme/jar.mn
@@ +95,5 @@
>    skin/images/unmute-hdpi.png               (images/unmute-hdpi.png)
>    skin/images/fullscreen-hdpi.png           (images/fullscreen-hdpi.png)
>    skin/images/exitfullscreen-hdpi.png       (images/exitfullscreen-hdpi.png)
>    skin/images/scrubber-hdpi.png             (images/scrubber-hdpi.png)
> +  skin/images/[e-mail]    (images/[e-mail])

Nit: I think we've been leaving off the "@1.0x" on the other 1x images.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Jonathan Wilde [:jwilde] from comment #19)
> Comment on attachment 789721 [details] [diff] [review]
> css
> 
> Review of attachment 789721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=jwilde with a few tweaks.
> 
> ::: browser/metro/theme/browser.css
> @@ +290,5 @@
> >  .selection-overlay-hidden {
> >    display: none;
> >  }
> >  
> >  .selectionhandle {
> 
> I think we need to set an explicit width/height on .selectionhandle. See
> .appbar-primary (and, more specifically, .appbar-primary >
> .toolbarbutton-icon) styles for reference.
> 
> Right now, it looks like the size of the image increases with the screen
> pixel width/height associated with the image, compounded by the
> device-screen pixel scaling factor. This makes the handles a little larger
> than we want, I think. :-)

ahh, I was wondering about that. How do you choose the right width/height? Make it the same as the base 1.0 image?

Comment 21

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 22

[Jim Mathies [:jimm]]

Attachment: patch v.2

This took a little more work than I expected. list-style-image wasn't working right for setting width and height so I had to convert to background-image. Also locally I was getting a couple random test failures in some selection tests, so I touched those up too.

Comment 23

[Jim Mathies [:jimm]]

Attachment: patch v.2

up to date patch file.

Comment 24

[Jonathan Wilde [:jwilde]]

Comment on attachment 790205 [details] [diff] [review]
patch v.2

Review of attachment 790205 [details] [diff] [review]:
-----------------------------------------------------------------

Hm. It looks like the selection monacle images weren't included with this patch file?

Comment 25

[Jonathan Wilde [:jwilde]]

Looking more into the patch file:

> diff -r 6ecfb2309c7a -r acb766c5f486 browser/metro/theme/images/selection-monocle.png
> Binary file browser/metro/theme/images/selection-monocle.png has changed
> diff -r 6ecfb2309c7a -r acb766c5f486 browser/metro/theme/images/[e-mail]
> Binary file browser/metro/theme/images/[e-mail] has changed

The files are certainly there as far as mq is concerned, but they're not actually getting dumped into the patch. Is the format set right in mq?

Comment 26

[Jim Mathies [:jimm]]

Attachment: patch v.2

odd, hg export tip didn't export the binary data, but hg qdiff does.

Comment 27

[Jonathan Wilde [:jwilde]]

Comment on attachment 790367 [details] [diff] [review]
patch v.2

Review of attachment 790367 [details] [diff] [review]:
-----------------------------------------------------------------

Kicking this to mbrubeck, as I'm short on cycles atm.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b9ec8cfec12