Set As Desktop Background does not work on OS X

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: Ehsan, Assigned: spohl)

Tracking

unspecified
Firefox 57
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
STR: Right click on an image and select "Set As Desktop Background".
tracking-e10s: ? → +

Updated

3 years ago
Duplicate of this bug: 1138235

Comment 2

3 years ago
Re-nomming. This is secondary UI, but it's going to be very visible if we roll out e10s to everyone and this doesn't work at all. The dupe says the button in the dialog is also broken so the functionality itself is just dead in the water.
tracking-e10s: + → ?
STR's:
1) Do a search for "images"
2) Right click any of the images to get the image context menu.
3) Click Set As Desktop Background... to get Preview dialog of image.
4) Click Set Desktop Background button

Expected result: 
The image is saved and the desktop background is set to the new image.

Tested result: 
The Set Desktop Background button changes to "Saving Picture..." but the image is not saved and the desktop background is never set to the new image.

Mac 10.9.5 Nightly 47.0a1 20160125060632 (e10s and non-e10s) - FAIL
Mac 10.9.5 Firefox Release 26.0 (non-e10s) - FAIL
Mac 10.9.5 Firefox Release 20.0.1 (non-e10s) - FAIL

Windows 7 Nightly 47.0a1 20160125060632 (e10s) - PASS
Ubuntu 14 Nightly 47.0a1 20160126030244 (e10s) - PASS

This has not worked correctly on Mac in a long time, if ever (see the old releases tested above). It works as expected on Windows and Linux. Note: The Windows and Linux Preview dialogs have additional options for Position (sizing) and Color.  Those dialogs also have a Cancel button.  None of those option are available in the Mac Preview dialog.

Comment 4

3 years ago
If we can't get this to work on mac we should remove the option.

Comment 5

3 years ago
FWIW, testing on OSX, the preview does show up, and it does so on Windows on e10s as well. I'll just morph this about making this work on OS X when actually doing the thing, then... :-)
tracking-e10s: ? → ---
Component: General → Shell Integration
Summary: Set As Desktop Background preview does not show up in e10s mode → Set As Desktop Background does not work on OS X
This still happens on Nightly, it seems.
And as Jim said, if we can't fix this, we need to remove it. We shouldn't be shipping features that just don't work.

I'm getting:

NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIShellService.setDesktopBackground]

which implies we're going down this path:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#12
So I was correct that we are going down that path, but I don't know why.

If I open

https://static.storied.co/JQl670PRVQPWKNk1/projects/w1Z975BX7O2nOBqm/assets/images/1022dc170e8769727e615489e1f07e7d_extra_large.jpg

In a page and select "Set as Desktop background", the image URI obtained here:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#112

Is a data URI, it is not the original URI (as I would have expected it to be).

So we end up going down this path:

http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#128
Whiteboard: tpi:?
I've tried some idea, but nothing is working. I don't really understand the code, so I'll have to punt.

What I've discovered:

1. Even if you get the HTMLImageElement and check the src, it's still a data URL.
2. Right clicking on an image and setting as desktop background still uses a data URL.
3. I can't use mozregression to track this down because old version of Firefox don't run on current Sierra due to bug 1286613.

So I think the key here is getting the real image URL, I just don't know how to get it.
(Assignee)

Comment 10

a year ago
(In reply to Mike Kaply [:mkaply] from comment #9)
> I've tried some idea, but nothing is working. I don't really understand the
> code, so I'll have to punt.

Thanks, Mike! I'm taking a look.
(Assignee)

Comment 13

a year ago
Created attachment 8894264 [details] [diff] [review]
Patch

This didn't work because we were trying to get the leaf name from the original URL of the image, but the URL in setDesktopBackground is a data URL. This patch passes the leaf name along to setDesktopBackground as an additional argument. This is ignored on Windows (where we retrieve a localized file name and convert the image to a .bmp) and Linux (where we generate the filename based on the brand name and convert the image to a .png).

Try run (comment 12) is green.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #8894264 - Flags: review?(mstange)
Comment on attachment 8894264 [details] [diff] [review]
Patch

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

::: browser/base/content/content.js
@@ +1117,5 @@
> +      }
> +      imageNameEnd = imageName.lastIndexOf("#");
> +      if (imageNameEnd > 0) {
> +        imageName = imageName.substr(0, imageNameEnd);
> +      }

(new URL(url)).pathname can save you some work here; you'll still need to cut off leading directories, though.

::: browser/components/shell/nsMacShellService.cpp
@@ +136,3 @@
>  
>    // and add the imgage file name itself:
>    mBackgroundFile->Append(fileNameUnicode);

What happens if the supplied filename is empty, too long, has a file extension that's different than the actual image format, or contains characters that are not valid within filenames?

I'm also a bit surprised that we just blindly overwrite an existing file with the same filename in ~/Pictures and don't make any effort to avoid collisions, but I suppose that's a different issue.

Comment 15

a year ago
Is there any chance we could add an automated test for this? Or is that silly levels of difficult?
Do we know what actually broke this? This doesn't sound like it would have ever worked? Did something change to always pass data URLs to set as background?
I'd like to get this done in this cycle, if possible.
(Assignee)

Comment 18

a year ago
Created attachment 8894635 [details] [diff] [review]
Patch

(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8894264 [details] [diff] [review]
> Patch
> 
> Review of attachment 8894264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/content.js
> @@ +1117,5 @@
> > +      }
> > +      imageNameEnd = imageName.lastIndexOf("#");
> > +      if (imageNameEnd > 0) {
> > +        imageName = imageName.substr(0, imageNameEnd);
> > +      }
> 
> (new URL(url)).pathname can save you some work here; you'll still need to
> cut off leading directories, though.

Done, thank you!

> ::: browser/components/shell/nsMacShellService.cpp
> @@ +136,3 @@
> >  
> >    // and add the imgage file name itself:
> >    mBackgroundFile->Append(fileNameUnicode);
> 
> What happens if:
> the supplied filename is empty,

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> too long,

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> has a file extension that's different than the actual image format,

I've experimentally been able to confirm that macOS does not seem to care about the file extension and will display the correct background if the image is of a valid image type for backgrounds. Even .txt works.

> or contains characters that are not valid within filenames?

Nothing. No file is created and the desktop background image remains unchanged. We fail here:
https://dxr.mozilla.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#2313

> I'm also a bit surprised that we just blindly overwrite an existing file
> with the same filename in ~/Pictures and don't make any effort to avoid
> collisions, but I suppose that's a different issue.

Right. I've basically kept the current behavior. Note that OSX is different from Linux and Windows here. On Windows and Linux, we create a background image with a specific filename. When the user chooses to change the background to a different image, this same file is overwritten. On OSX, the different files persist on the system unless there is a name collision.
Attachment #8894264 - Attachment is obsolete: true
Attachment #8894264 - Flags: review?(mstange)
Attachment #8894635 - Flags: review?(mstange)
(Assignee)

Comment 19

a year ago
(In reply to Mike Kaply [:mkaply] from comment #16)
> Do we know what actually broke this? This doesn't sound like it would have
> ever worked? Did something change to always pass data URLs to set as
> background?

Still looking into it.

(In reply to :Gijs from comment #15)
> Is there any chance we could add an automated test for this? Or is that
> silly levels of difficult?

Trying to write one now.
(Assignee)

Comment 20

a year ago
(In reply to Mike Kaply [:mkaply] from comment #16)
> Do we know what actually broke this? This doesn't sound like it would have
> ever worked? Did something change to always pass data URLs to set as
> background?

Okay, I was finally able to track this down. This initially broke when we enabled e10s but continued to work in non-e10s. That's when this bug was filed. This eventually broke completely on macOS/OSX when we attempted to fix it in bug 1135933 for all platforms by switching to data URLs. Although the preview would now show an image (which it failed to do in e10s before), it was no longer able to save the image because macOS/OSX never supported saving background images from data URLs.
Blocks: 1135933
(Assignee)

Comment 22

a year ago
Created attachment 8895000 [details] [diff] [review]
Tests

This adds a test for macOS and updates an existing test for Linux. Try run in comment 21.
Attachment #8895000 - Flags: review?(mstange)
Comment on attachment 8894635 [details] [diff] [review]
Patch

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

Should probably get a quick review from a frontend person as well, for the JavaScript parts.
Attachment #8894635 - Flags: review?(mstange)
Attachment #8894635 - Flags: review?(mconley)
Attachment #8894635 - Flags: review+
Comment on attachment 8894635 [details] [diff] [review]
Patch

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

Thanks!

::: browser/base/content/content.js
@@ +1114,2 @@
>        sendAsyncMessage("ContextMenu:SetAsDesktopBackground:Result",
> +                       { dataUrl, imageName});

Nit - space before }
Attachment #8894635 - Flags: review?(mconley) → review+
Comment on attachment 8895000 [details] [diff] [review]
Tests

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

::: browser/components/shell/test/browser_1119088.js
@@ +8,5 @@
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIDirectoryServiceProvider);
> +
> +  let desktopBackgroundDb = dirSvc.getFile(NS_MAC_USER_LIB_DIR, {});
> +  desktopBackgroundDb.append("Application\ Support");

I think an escaped space in a JavaScript string literal is just a space. The backslash doesn't actually go into the string (you'd need \\ for that), and it's not necessary.

@@ +48,5 @@
> +    let dockArg = ["Dock"];
> +    let process =
> +      Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
> +    process.init(killall);
> +    process.run(true, dockArg, 1);

Oh wow, that's intense. I find it hard to predict whether that's going to cause problems on our test machines. I suppose we can still back this out if we start seeing strange issues.

@@ +58,5 @@
> +
> +function test() {
> +  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> +  gBrowser.selectedBrowser.addEventListener("load", onPageLoad, true);
> +  content.location = "about:logo";

Nice choice of picture.
Attachment #8895000 - Flags: review?(mstange) → review+
(Assignee)

Comment 27

a year ago
Created attachment 8895203 [details] [diff] [review]
Patch

Thanks for the reviews! Addressed nit. Carrying over r+.
Attachment #8894635 - Attachment is obsolete: true
Attachment #8895203 - Flags: review+
(Assignee)

Comment 28

a year ago
Created attachment 8895207 [details] [diff] [review]
Tests

Removed escaped space, thanks!

I think I messed up browser.ini in the previous patch because my new macOS-only test ran on Linux on try (see comment 21). I've launched a new try run (see comment 26) and Linux looks good now. Still waiting for macOS tests to run. Markus, would you mind taking one last look at these changes? Thanks!

(In reply to Markus Stange [:mstange] from comment #25)
> Comment on attachment 8895000 [details] [diff] [review]
> Tests
> 
> Review of attachment 8895000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/shell/test/browser_1119088.js
> @@ +48,5 @@
> > +    let dockArg = ["Dock"];
> > +    let process =
> > +      Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
> > +    process.init(killall);
> > +    process.run(true, dockArg, 1);
> 
> Oh wow, that's intense. I find it hard to predict whether that's going to
> cause problems on our test machines. I suppose we can still back this out if
> we start seeing strange issues.

Yes, I completely agree. I'm hoping this will stick. Otherwise, we may be able to load the database file as a sql database and determine the current background image before running the test. However, I'm not sure this is documented and I suppose the format of the database file could change.
Attachment #8895000 - Attachment is obsolete: true
Attachment #8895207 - Flags: review?(mstange)
(Assignee)

Comment 29

a year ago
(In reply to Stephen A Pohl [:spohl] from comment #28)
> I think I messed up browser.ini in the previous patch because my new
> macOS-only test ran on Linux on try (see comment 21). I've launched a new
> try run (see comment 26) and Linux looks good now. Still waiting for macOS
> tests to run.

The try run is green. I've confirmed that the test only ran on macOS and it did so successfully.
Comment on attachment 8895207 [details] [diff] [review]
Tests

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

Yeah, let's not depend on the database format.
Attachment #8895207 - Flags: review?(mstange) → review+
(Assignee)

Comment 31

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59e491465e89c549f21d8bf22b9d8725198412b
Bug 1119088: Fix ability to set an image as desktop background via context menu on macOS. r=mstange,mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/4da996b87a5e3d89c487fa5848e24b23c6f90080
Bug 1119088: Update test for setDesktopBackground() and add test for macOS. r=mstange

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e59e491465e8
https://hg.mozilla.org/mozilla-central/rev/4da996b87a5e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

a year ago
Whiteboard: tpi:? → tpi:+

Comment 33

9 months ago
Does this mean bug 820515 is also fixed? :-)
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 34

9 months ago
(In reply to :Gijs from comment #33)
> Does this mean bug 820515 is also fixed? :-)

It does indeed! Thanks for pointing this out.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Updated

9 months ago
Duplicate of this bug: 820515
You need to log in before you can comment on or make changes to this bug.