Work - Update selection monocle image

RESOLVED FIXED in Firefox 26

Status

P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 26
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work [preview])

Attachments

(5 attachments, 8 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 682485 [details]
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.
(Assignee)

Updated

6 years ago
OS: Windows 7 → Windows 8 Metro
Assignee: nobody → shorlander
Keywords: uiwanted
(Assignee)

Updated

6 years ago
Blocks: 781487

Updated

6 years ago
Blocks: 831908, 831909
Whiteboard: [metro-mvp] → [metro-mvp] feature=work

Updated

6 years ago
Summary: Update selection monocle image → Work - Update selection monocle image
Whiteboard: [metro-mvp] feature=work → feature=work
(Assignee)

Updated

6 years ago
No longer blocks: 781487
(Assignee)

Updated

6 years ago
Blocks: 788000
No longer blocks: 831908

Updated

6 years ago
Blocks: 838497
(Assignee)

Updated

6 years ago
No longer blocks: 788000

Updated

6 years ago
Priority: -- → P5

Updated

6 years ago
Priority: P5 → P4

Updated

6 years ago
Priority: P4 → P1
Created attachment 756117 [details]
Selection Monocle

The current size is an unusual 32x34, not sure if that is intentional. This is 18 x 18.
Created attachment 756119 [details]
Selection Monocle - 140%
Created attachment 756120 [details]
Selection Monocle - 180%
(Assignee)

Updated

6 years ago
Assignee: shorlander → jmathies
(Assignee)

Updated

6 years ago
No longer blocks: 838497
(Assignee)

Comment 4

6 years ago
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)

Comment 5

5 years ago
(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)

Comment 6

5 years ago
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");
    }
}
(Assignee)

Comment 7

5 years ago
Created attachment 761993 [details] [diff] [review]
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

5 years ago
(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).
(Assignee)

Updated

5 years ago
Depends on: 883405
(Assignee)

Updated

5 years ago
Whiteboard: feature=work → [leave-open] feature=work
(Assignee)

Comment 9

5 years ago
Created attachment 763519 [details] [diff] [review]
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.
Attachment #761993 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 763529 [details] [diff] [review]
images and css
(Assignee)

Comment 11

5 years ago
Created attachment 763560 [details] [diff] [review]
widget dpi fix v.1

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)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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)

Updated

5 years ago
Assignee: jmathies → nobody
(Assignee)

Updated

5 years ago
Whiteboard: [leave-open] feature=work [preview-triage] → [leave-open] feature=work [preview]
(Assignee)

Updated

5 years ago
Assignee: nobody → jmathies
Whiteboard: [leave-open] feature=work [preview] → feature=work [preview]
(Assignee)

Comment 18

5 years ago
Created attachment 789721 [details] [diff] [review]
css
Attachment #789721 - Flags: review?(jwilde)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 20

5 years ago
(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?
(Assignee)

Comment 21

5 years ago
Created attachment 790203 [details] [diff] [review]
patch v.2
Attachment #763560 - Attachment is obsolete: true
Attachment #789721 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 790204 [details] [diff] [review]
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.
Attachment #790203 - Attachment is obsolete: true
Attachment #790204 - Flags: review?(jwilde)
(Assignee)

Comment 23

5 years ago
Created attachment 790205 [details] [diff] [review]
patch v.2

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?
(Assignee)

Comment 26

5 years ago
Created attachment 790367 [details] [diff] [review]
patch v.2

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
Last Resolved: 5 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.