632585 - Port Firefox Set as wallpaper dialog to Seamonkey.

Status: RESOLVED FIXED

(SeaMonkey :: OS Integration, enhancement)

Description

[Matt A. Tobin [:nsITobin]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre SeaMonkey/2.1b3pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre SeaMonkey/2.1b3pre

Firefox has a very nice "Set image as wallpaper" dialog where you can select the mode such as stretch or center and it shows a preview.

Seamonkey simply as a yes or no style message window.

Reproducible: Always

Steps to Reproduce:
1. Find an image.
2. Right click and select set as wallpaper.

Actual Results:  
Message window pops up asking set or not to set image as wallpaper.

Expected Results:  
User should be granted a preview and tiling mode of the wallpaper the way Firefox does.

Comment 1

[Matt A. Tobin [:nsITobin]]

Attachment: Screenshot of the difference between Seamonkey and Firefox regarding this bug.

Comment 2

[Philip Chee]

Related Firefox bugs:
Bug 274374 - Mac OS X Shell Service ("Set default browser" support, etc.).
Bug 298555 - "Set as desktop background" does not work.
Bug 296583 - Make "Set Desktop Background" dialog skinable.
Bug 263473 - Context menu displays all possible options on image <object>. Also makes "Set Desktop Background" work with <object> elements and fixes bug 305380.
Bug 262134 - Set as Desktop Background (wallpaper) gets zoomed (scaled) image dimensions instead of real.
ones.
Bug 362239 - Replace 'desktop background' monitor image on Windows with something nicer.
Bug 385844 - Set Desktop Background: implement preview for image tiling.
Bug 386163 - 'Set Desktop Background' refactoring: use canvas in all cases, support widescreen previews.

Comment 3

[Stanimir Stamenkov]

I'll look into it, although it appears a bit more advanced for my skills and I won't be able to get to it until the weekend.  I won't assign myself immediately, until I get something to present for review, so others are free to take it, also.

Comment 4

[Stanimir Stamenkov]

As discussed over IRC - this will need to port some C++ changes over the three platforms, and I'm only able to build on Windows.

Comment 5

[[e-mail]]

Attachment: Proposed patch

r?IanN for the code and operation on Linux
r?Ratty for the operation on Windows
f?KaiRo to check that I've localised it correctly
f?Serge to check whether I need any test changes

Comment 6

[Philip Chee]

> f?Serge to check whether I need any test changes
I'm not Serge but you'll certainly break /suite/browser/test/mochitest/test_contextmenu.html :

http://mxr.mozilla.org/comm-central/search?string=context-setWallpaper&find=test_contextmenu

Comment 7

[Philip Chee]

> +      <menuitem id="context-setBackground"
> +                label="&setBackgroundCmd.label;"
> +                accesskey="&setBackgroundCmd.accesskey;"
> +                oncommand="gContextMenu.setBackground();"/>

[Could we perhaps have the id as "context-setDesktopBackground" like Firefox? Makes it easier to port Firefox extensions.]

> -    this.showItem("context-setWallpaper",
> +    this.showItem("context-setBackground",

You need to change test_contextmenu.html as well.

> -<!ENTITY setWallpaperCmd.label        "Set As Wallpaper">
> -<!ENTITY setWallpaperCmd.accesskey    "A">
> +<!ENTITY setBackgroundCmd.label       "Set Desktop Background…">
> +<!ENTITY setBackgroundCmd.accesskey   "D">
[Any reason this isn't "Set *As* Desktop Background…"]

> +  } catch (e) {
> +    gPicker.disabled = true;
> +    gPicker.previousSibling.disabled = true;
[I don't suppose an observer child on the label would work?]

> +<!ENTITY accept.label               "Save">

While technically correct, I think this is confusing to the user. They don't want to "Save" anything, just set an image as wallpaper. Unfortunately I can't think of anything better, "OK", "Do It", "Make It So". "OK" might be better.

One strange quirk I encountered when testing this patch. STR:
1. Right click on an Image, Select Set Desktop Background.
2. Dialog opens with the mouse cursor focused on the [Save] button.
3. Move the mouse cursor into the preview area.
4. The image blinks off for a couple of seconds (leaving only the background colour), then blinks back on.

It's rather unnerving.

r=me if you fix the test and the blanking preview.

Comment 8

[[e-mail]]

(In reply to Philip Chee from comment #7)
> > +      <menuitem id="context-setBackground"
> > +                label="&setBackgroundCmd.label;"
> > +                accesskey="&setBackgroundCmd.accesskey;"
> > +                oncommand="gContextMenu.setBackground();"/>
> [Could we perhaps have the id as "context-setDesktopBackground" like
> Firefox? Makes it easier to port Firefox extensions.]
I can't remember whether I looked at what Firefox did, I probably just wanted to save some typing ;-)

> > -<!ENTITY setWallpaperCmd.label        "Set As Wallpaper">
> > -<!ENTITY setWallpaperCmd.accesskey    "A">
> > +<!ENTITY setBackgroundCmd.label       "Set Desktop Background…">
> > +<!ENTITY setBackgroundCmd.accesskey   "D">
> [Any reason this isn't "Set *As* Desktop Background…"]
Because it opens the "Set Desktop Background" dialog? Also, it's short for "Set as wallpaper and also change the desktop background colour at the same time."

> > +  } catch (e) {
> > +    gPicker.disabled = true;
> > +    gPicker.previousSibling.disabled = true;
> [I don't suppose an observer child on the label would work?]
Good idea, I'll give that a whirl.

> > +<!ENTITY accept.label               "Save">
> While technically correct, I think this is confusing to the user.
Yes, it was the best I could think of without repeating "Set Desktop Background" a third time...

> One strange quirk I encountered when testing this patch. STR:
> 1. Right click on an Image, Select Set Desktop Background.
> 2. Dialog opens with the mouse cursor focused on the [Save] button.
> 3. Move the mouse cursor into the preview area.
> 4. The image blinks off for a couple of seconds (leaving only the background
> colour), then blinks back on.
> 
> It's rather unnerving.
> 
> r=me if you fix the test and the blanking preview.
Can't reproduce, sorry :-(

Comment 9

[Philip Chee]

I noticed this in the Firefox code:

> #ifdef XP_WIN
>     // hide fill + fit options if <win7 since don't work 
>     var version = Components.classes["@mozilla.org/system-info;1"]
>                   .getService(Ci.nsIPropertyBag2)
>                   .getProperty("version");
>     var isWindows7OrHigher = (parseFloat(version) >= 6.1);
>     if (!isWindows7OrHigher) {
>       document.getElementById("fillPosition").hidden = true;
>       document.getElementById("fitPosition").hidden = true;
>     }
> #endif

q.v. Bug 662324 - Add "Fill" and "Fit" position support for "Set As Desktop Background..."

A bit of googling indicates that:

Picture position options in Windows are:

* Fill - only in Windows 7 and 8, picture is resized to screen resolution while retaining aspect ratio
* Fit or Fit to Screen - in Windows Vista, picture is resized to screen resolution ignoring aspect ratio. In Windows 7 and 8, the aspect ratio is retained, possibly leaving blank edges on the screen.
* Stretch - in Windows XP, 7 and 8, picture is resized to screen resolution without retaining aspect ratio.
* Tile - if a picture is smaller than screen resolution, it is repeated across the screen vertically and horizontally.
* Center - if a picture is smaller than screen resolution, it is centered on the screen, leaving blank areas around it. If a photo is larger than screen resolution, only center part of it is visible.

Comment 10

[[e-mail]]

(In reply to Philip Chee from comment #7)
> > +<!ENTITY accept.label               "Save">
> While technically correct, I think this is confusing to the user.
How about "Apply" and "Close"?

Comment 11

[Ian Neal]

Comment on attachment 643589 [details] [diff] [review]
Proposed patch

>+++ b/suite/common/setDesktopBackground.xul
>+<dialog id="setDesktopBackground"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        onload="onLoad();"
>+        ondialogaccept="onAccept();"
>+        buttonlabelaccept="&accept.label;"
Accesskeys?

>+  <hbox align="center">
>+    <label value="&position.label;"/>
Accesskey?

>+    <label value="&picker.label;"/>
Accesskey?
>+    <colorpicker id="picker" type="button" onchange="updateColor();"/>
On linux this is disabled until it is implemented but should it be hidden for OSX?

>+++ b/suite/locales/en-US/chrome/common/contentAreaCommands.dtd
>-<!ENTITY setWallpaperCmd.label        "Set As Wallpaper">
>-<!ENTITY setWallpaperCmd.accesskey    "A">
>+<!ENTITY setBackgroundCmd.label       "Set Desktop Backgroundâ¦">
>+<!ENTITY setBackgroundCmd.accesskey   "D">
D clashes with the accesskey for Send This Page, perhaps leave it as "a"?

Should this menu item be appearing for a message pane?

Just wondering why the Firefox version looks so complicated compared to this.
r=me with issues addressed/answered

Comment 12

[[e-mail]]

(In reply to Ian Neal from comment #11)
> (From update of attachment 643589 [details] [diff] [review])
> >+        buttonlabelaccept="&accept.label;"
> Accesskeys?
Accept/cancel buttons don't normally need them, because you can use Enter/Esc.

> >+    <label value="&position.label;"/>
> >+    <label value="&picker.label;"/>
> Accesskey?
Good point.

> >+    <colorpicker id="picker" type="button" onchange="updateColor();"/>
> On linux this is disabled until it is implemented but should it be hidden
> for OSX?
We don't support Set Desktop Background on OSX anyway. If we did then I would advise the implementor to add support for the picker if possible. I'm no OSX programmer but there appear to be some newer APIs that Firefox doesn't use yet which look as if they would support the other options.

> >+<!ENTITY setBackgroundCmd.accesskey   "D">
> D clashes with the accesskey for Send This Page, perhaps leave it as "a"?
Oops, I'd only tried this with linked images. I'll change it back.

> Should this menu item be appearing for a message pane?
It wasn't before, so...

> Just wondering why the Firefox version looks so complicated compared to this.
background-size hadn't been invented back then.

Comment 13

[Ian Neal]

(In reply to [e-mail] from comment #12)
> (In reply to Ian Neal from comment #11)
> > (From update of attachment 643589 [details] [diff] [review])
> > >+        buttonlabelaccept="&accept.label;"
> > Accesskeys?
> Accept/cancel buttons don't normally need them, because you can use
> Enter/Esc.
Fine.
> > >+    <colorpicker id="picker" type="button" onchange="updateColor();"/>
> > On linux this is disabled until it is implemented but should it be hidden
> > for OSX?
> We don't support Set Desktop Background on OSX anyway. If we did then I
> would advise the implementor to add support for the picker if possible. I'm
> no OSX programmer but there appear to be some newer APIs that Firefox
> doesn't use yet which look as if they would support the other options.
So should we be disabling or hiding for the platforms it is not yet implemented on?

> > Should this menu item be appearing for a message pane?
> It wasn't before, so...
...outside of scope then.
> 
> > Just wondering why the Firefox version looks so complicated compared to this.
> background-size hadn't been invented back then.
Would that potentially be Bug 253197 for them?

Comment 14

[Philip Chee]

> Would that potentially be Bug 253197 for them?
It's WFM in Firefox probably due to their rewrite to use canvas.

Comment 15

[Robert Kaiser]

Comment on attachment 643589 [details] [diff] [review]
Proposed patch

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

Looks good to me L10n-wise.

Comment 16

[Philip Chee]

> So should we be disabling or hiding for the platforms it is not yet implemented on?
We already do that. In nsContextMenu we hide the menu item for unsupported platforms.

Comment 17

[Ian Neal]

(In reply to Philip Chee from comment #16)
> > So should we be disabling or hiding for the platforms it is not yet implemented on?
> We already do that. In nsContextMenu we hide the menu item for unsupported
> platforms.
I was talking about the colour picker in the dialog box.

Comment 18

[[e-mail]]

Attachment: Addressed review comments

Comment 19

[Philip Chee]

Comment on attachment 645451 [details] [diff] [review]
Addressed review comments

$ MOZ_NO_REMOTE=1 TEST_PATH=suite/browser/test/test_contextmenu.html  pymake -C ../objdir-sm/ mochitest-plain

Passed: 1710
Failed: 3
Todo: 0
failed | checking item #9 (context-DesktopBackground) name - got context-setDesktopBackground, expected context-DesktopBackground
failed | checking item #17 (context-DesktopBackground) name - got context-setDesktopBackground, expected context-DesktopBackground
failed | checking item #12 (context-DesktopBackground) name - got context-setDesktopBackground, expected context-DesktopBackground

Spot the mistake:

> -                         ["context-setWallpaper", true] : []).concat(
> +                         ["context-setDesktopBackground", true] : []).concat(
> 
> -                         ["context-setWallpaper", true] : [])
> +                         ["context-DesktopBackground", true] : [])
> 
> -                         ["context-setWallpaper", true] : []).concat(
> +                         ["context-DesktopBackground", true] : []).concat(
> 
> -                         ["context-setWallpaper", true] : []).concat(
> +                         ["context-DesktopBackground", true] : []).concat(

The new dialog has the Accept button on the bottom left and the Close button on the bottom right with a large space in between. I don't like this visually.

Also clicking Accept does not dismiss the dialog.

> +  try {
> +    var color = gShell.desktopBackgroundColor;
> +    color = (0xF000000 | color).toString(16).toUpperCase().replace("F", "#");
> +    gDesktop.style.backgroundColor = color;
> +    gPicker.color = color;
> +  } catch (e) {
> +    gPicker.parentNode.hidden = true;
> +  }

[would it be better to set XUL element to hidden and then unhide it within the try block?]

Comment 20

[[e-mail]]

(In reply to Philip Chee from comment #19)
> Spot the mistake:
> > -                         ["context-setWallpaper", true] : []).concat(
> > +                         ["context-setDesktopBackground", true] : []).concat(
I typed this one manually, then tried to copy and paste the others, but oops.

> The new dialog has the Accept button on the bottom left and the Close button
> on the bottom right with a large space in between. I don't like this
> visually.
> 
> Also clicking Accept does not dismiss the dialog.
Ah yes, I really should have renamed it onApply to match the label...

Comment 21

[Ian Neal]

Comment on attachment 645451 [details] [diff] [review]
Addressed review comments

r=me with onAccept->onApply and tests fixed.

Comment 22

[[e-mail]]

(In reply to Philip Chee from comment #19)
> The new dialog has the Accept button on the bottom left and the Close button
> on the bottom right with a large space in between. I don't like this visually.
It has the Position on the top left and the Colour on the top right with a large space between. Do you not like that visually either? I can't really put them all on one line because different platforms have different button order conventions, but I could change it from a dialog to a window (so that it would have a close button) and then put Position, Colour and Apply on one line?

> [would it be better to set XUL element to hidden and then unhide it within
> the try block?]
Most people will have the element unhidden. (Also XBL used to work better if the element is not unhidden when the window loads, although that may have since been fixed.)

Comment 23

[[e-mail]]

Attachment: Fixed patch

Comment 24

[Philip Chee]

Comment on attachment 647024 [details] [diff] [review]
Fixed patch

Test passes!
I guess I can live with this UI.
r=me

Comment 25

[[e-mail]]

Pushed comm-centra changeset 17e63702fc69.