Closed Bug 812529 Opened 8 years ago Closed 8 years ago

Work - Update selection monocle image

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: feature=work [preview])

Attachments

(5 files, 8 obsolete files)

Attached image 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.
OS: Windows 7 → Windows 8 Metro
Blocks: 781487
Blocks: 831908, 831909
Whiteboard: [metro-mvp] → [metro-mvp] feature=work
Summary: Update selection monocle image → Work - Update selection monocle image
Whiteboard: [metro-mvp] feature=work → feature=work
No longer blocks: 781487
Blocks: 788000
No longer blocks: 831908
No longer blocks: 788000
Priority: -- → P5
Priority: P5 → P4
Priority: P4 → P1
Attached image Selection Monocle
The current size is an unusual 32x34, not sure if that is intentional. This is 18 x 18.
Assignee: shorlander → jmathies
No longer blocks: Backlog-MetroDesign
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.
Flags: needinfo?(fyan)
(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/selection-monocle@1.4x.png");
        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/selection-monocle@1.8x.png");
    }
}

Resource: http://msdn.microsoft.com/en-us/library/windows/apps/hh465362.aspx
Flags: needinfo?(fyan)
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/selection-monocle@1.4x.png");
        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/selection-monocle@1.8x.png");
    }
}
Attached patch patch (obsolete) — Splinter Review
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.
(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).
Depends on: 883405
Whiteboard: feature=work → [leave-open] feature=work
Attached patch widget dpi fix v.1 (obsolete) — Splinter Review
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.
Attachment #761993 - Attachment is obsolete: true
Attached patch images and css (obsolete) — Splinter Review
Attached patch widget dpi fix v.1 (obsolete) — Splinter Review
Looks ok, reports 96 on our test slaves, 135 on the surface pro. I also added some caching.
Attachment #763519 - Attachment is obsolete: true
Attachment #763560 - Flags: review?(tabraldes)
No longer depends on: 883405
Duplicate of this bug: 883405
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.
Attachment #763560 - Flags: review?(tabraldes) → review+
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.
(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.
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.
Kicking this over to the hidpi assets story so I can close out chrome selection work.
Blocks: 833192
No longer blocks: 831909
Whiteboard: [leave-open] feature=work → [leave-open] feature=work [preview-triage]
Assignee: jmathies → nobody
Whiteboard: [leave-open] feature=work [preview-triage] → [leave-open] feature=work [preview]
Assignee: nobody → jmathies
Whiteboard: [leave-open] feature=work [preview] → feature=work [preview]
Attached patch css (obsolete) — Splinter Review
Attachment #789721 - Flags: review?(jwilde)
Attachment #763529 - Attachment is obsolete: true
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/selection-monocle@1.0x.png    (images/selection-monocle@1.0x.png)

Nit: I think we've been leaving off the "@1.0x" on the other 1x images.
Attachment #789721 - Flags: review?(jwilde) → review+
(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?
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #763560 - Attachment is obsolete: true
Attachment #789721 - Attachment is obsolete: true
Attached patch patch v.2 (obsolete) — Splinter Review
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.
Attachment #790203 - Attachment is obsolete: true
Attachment #790204 - Flags: review?(jwilde)
Attached patch patch v.2 (obsolete) — Splinter Review
up to date patch file.
Attachment #790204 - Attachment is obsolete: true
Attachment #790204 - Flags: review?(jwilde)
Attachment #790205 - Flags: review?(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?
Attachment #790205 - Flags: review?(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/selection-monocle@1.4x.png
> Binary file browser/metro/theme/images/selection-monocle@1.4x.png 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?
Attached patch patch v.2Splinter Review
odd, hg export tip didn't export the binary data, but hg qdiff does.
Attachment #790205 - Attachment is obsolete: true
Attachment #790367 - Flags: review?(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.
Attachment #790367 - Flags: review?(jwilde) → review?(mbrubeck)
Attachment #790367 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/7b9ec8cfec12
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.