Port Firefox Set as wallpaper dialog to Seamonkey.

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: BinOC, Assigned: neil)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Status: UNCONFIRMED → NEW
Component: General → OS Integration
Ever confirmed: true
QA Contact: general → os-integration
Version: unspecified → Trunk
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.
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.
Assignee: nobody → stanio
Status: NEW → ASSIGNED
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.
Assignee: stanio → nobody
Status: ASSIGNED → NEW
Depends on: 771941
Blocks: 215532
Blocks: 127206
No longer blocks: 215532
Depends on: 774130
Posted patch Proposed patch (obsolete) — Splinter Review
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
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #643589 - Flags: review?(philip.chee)
Attachment #643589 - Flags: review?(iann_bugzilla)
Attachment #643589 - Flags: feedback?(sgautherie.bz)
Attachment #643589 - Flags: feedback?(kairo)
> 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
> +      <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.
(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 :-(
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.
(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 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
Attachment #643589 - Flags: review?(iann_bugzilla) → review+
(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.
(In reply to neil@parkwaycc.co.uk 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?
> Would that potentially be Bug 253197 for them?
It's WFM in Firefox probably due to their rewrite to use canvas.
Comment on attachment 643589 [details] [diff] [review]
Proposed patch

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

Looks good to me L10n-wise.
Attachment #643589 - Flags: feedback?(kairo) → feedback+
> 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.
(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.
Posted patch Addressed review comments (obsolete) — Splinter Review
Attachment #645451 - Flags: review?(philip.chee)
Attachment #645451 - Flags: review?(iann_bugzilla)
Attachment #643589 - Attachment is obsolete: true
Attachment #643589 - Flags: review?(philip.chee)
Attachment #643589 - Flags: feedback?(sgautherie.bz)
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?]
Attachment #645451 - Flags: review?(philip.chee) → review-
(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 on attachment 645451 [details] [diff] [review]
Addressed review comments

r=me with onAccept->onApply and tests fixed.
Attachment #645451 - Flags: review?(iann_bugzilla) → review+
(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.)
Posted patch Fixed patchSplinter Review
Attachment #645451 - Attachment is obsolete: true
Attachment #647024 - Flags: review?(philip.chee)
Comment on attachment 647024 [details] [diff] [review]
Fixed patch

Test passes!
I guess I can live with this UI.
r=me
Attachment #647024 - Flags: review?(philip.chee) → review+
Pushed comm-centra changeset 17e63702fc69.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 779300
No longer blocks: 127206
You need to log in before you can comment on or make changes to this bug.