965199 - Add "Copy as data URI" for images in the Inspector's CSS panels

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See bug 964014.
Filing this bug to do the same for the CSS rule-view and computed-view panels.

Comment 1

[Patrick Brosset <:pbro>]

The goal of this bug is to add a new item in the contextual menu that's shown when right-clicking on an image URL in the rules and computed view panels. This item should read something like "copy image as data-uri", and when clicked, the data-uri for the image should be stored in the user clipboard.
There's already a lot of code related to this in the devtools, so this shouldn't be hard to fix.

Comment 2

[Julian Descottes]

@patrick : I would like to work on this one, can you assign it to me ?

Comment 3

[Patrick Brosset <:pbro>]

A few, high-level, pointers: this is about the couple of panels we call the "style-inspector": /browser/devtools/styleinspector.

There are 2 panels in this tool, one we call the rule-view and the other the computed-view.
The rule-view contains the list of css rules (ordered by specificity) applied to the currently selected element.
The computed-view contains the full list of computed styles for the currently selected element.
Both panels are accessible in the inspector sidebar.
Both panels show css properties, and therefore, both panels may show "url(some-image-url)" values.

There are contextual menus defined in both views, and the idea is that these menus would contain a "copy as data-uri" item when the user right-clicks on an image url.

Worth knowing:
- the markup-view (the panel that contains the DOM tree in the inspector) already has such a feature, so does the network monitor, so you should have a look at them both to learn how is the image resolved to a data-uri string on the devtools server
- there may be problems with xbl bindings, for instance, open the browser toolbox, then the inspector in there, then select the #urlbar element (the address bar), and in the rule-view, you should see a moz-binding property that has a url to a xml file (-moz-binding: url("chrome://browser/content/urlbarBindings.xml#urlbar")). This looks exactly like a background image url, but isn't, so the menu should either not be here for those, or at least selecting it should fail nicely.

Comment 4

[Julian Descottes]

Thanks for the insights !

I only started scratching the surface:

Added the labels in styleinspector.properties (need to rebuild toolkit/locales).

I'm using the 'copy color' menu entry as a reference here, since it's displayed under certain circumstances. 

@Patrick : There's some code duplication between the computed-view and rule-view. Should I do something about this ? Or just make sure I don't duplicate the new code (and file a bug) ? Or we want to keep them as is ?

Comment 5

[Patrick Brosset <:pbro>]

(In reply to julian.descottes from comment #4)
> @Patrick : There's some code duplication between the computed-view and
> rule-view. Should I do something about this ? Or just make sure I don't
> duplicate the new code (and file a bug) ? Or we want to keep them as is ?
Filing a new bug to remove the code duplication for the building/handling/updating of the contextmenu in the rule/computed views would be great. Can you do it?

I don't think this new bug should necessarily be done before this one though. We don't want to keep maintaining duplicated code, that's for sure, but if the changes here are simple enough and exactly the same between the rule-view and computed-view, it would be easy enough to add a comment around it saying the duplication will go away when the other bug gets resolved.

Of course, if you're interested in getting rid of the duplication first and tackle the other bug, that's be fine too! I can find a mentor for it.

Comment 6

[Julian Descottes]

Should have a first patch ready for review later today.

> there may be problems with xbl bindings

For now I don't distinguish between image links and xbl binding links. This means that the context menu entry is displayed, but doesn't do anything. The image preview does the same, it displays an empty preview (with 'Invalid data' in it I think). 

Could be a good idea to put 'Invalid image data' in the clipboard in this case ? And do this for any broken image link ?

Some thoughts on filtering xbl binding links out : 

When the context menu is displayed, I only check and keep a reference to the image url, if any. When clicking on 'Copy image as data URI' then I do a server call to fetch the image data.

I could fetch the image data earlier, ie. when displaying the context menu. This would allow me to check if the link is really an image link, and disable the entry if needed. But this means doing a (probably unnecessary) server call every time the context menu is displayed on a link. Not sure about the performance impact. Or I could check such links using a regexp, but I fear this will be bug prone.

Not in favor of adding extra complexity for this.

Comment 7

[Julian Descottes]

> Filing a new bug to remove the code duplication for the building/handling/updating of the contextmenu
> in the rule/computed views would be great. Can you do it?

Was about to, but bugzilla pointed me to another bug you opened : bug 1134551 . Not too similar ?

Comment 8

[Patrick Brosset <:pbro>]

(In reply to julian.descottes from comment #7)
> > Filing a new bug to remove the code duplication for the building/handling/updating of the contextmenu
> > in the rule/computed views would be great. Can you do it?
> 
> Was about to, but bugzilla pointed me to another bug you opened : bug
> 1134551 . Not too similar ?
No I think these can remain as 2 separate bugs.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to julian.descottes from comment #6)
> Could be a good idea to put 'Invalid image data' in the clipboard in this
> case ? And do this for any broken image link ?
Why not, if it's simple enough.

> I could fetch the image data earlier, ie. when displaying the context menu.
> This would allow me to check if the link is really an image link, and
> disable the entry if needed. But this means doing a (probably unnecessary)
> server call every time the context menu is displayed on a link. Not sure
> about the performance impact.
Yeah, you're right, let's try to avoid this extra round-trip. Performance will usually not be a problem when debugging a local desktop tab, but may become one when debugging a firefoxos app over USB, or a page with a lot of javascript processing, especially if the image is big.
So for now, let's show the menu item whenever we click on something that looks like a URL.

> Or I could check such links using a regexp,
> but I fear this will be bug prone.
We're in the process of getting rid of *a lot* of the regexps we used to have for parsing css in favor of using the (now exposed to js) gecko css lexer (see bug 1152033, bug 1153305, bug 1154809), so let's not introduce a new one to determine if what we've clicked on is a URL to an image. We know it's a URL, that's enough for this bug. I agree let's not add too much complexity here. We could then add in a second step some more logic to differentiate URLs to images from URLs to other types of resources. I don't think there are so many css properties that can accept URLs that aren't for images, so that should be doable.

Comment 10

[Julian Descottes]

Somehow off-topic, but I have a question regarding the clipboardHelper. 

When calling clipboardHelper.copyString, the rule-view panel is wrongly passing undefined as second argument (supposed to be nsIDOMDocument*). But the clipboard is still filled with the expected text. 

From what I understood in nsClipboardHelper.cpp & nsTransferable.ccp a missing document means the transferable will assume the browser is not in private mode. However I don't see why this is needed for clipboard actions ? Can private browsing have an impact on the clipboard ?

Flagging @ehsan ?needinfo : you worked on nsClipboardHelper.cpp, maybe you can help me here or tell me who I should ask ?

Comment 11

[Julian Descottes]

Attachment: bug965199.patch

A first proposal for this issue !

As discussed there is a lot of code duplication between the 2 panels. This will be adressed by Bug 1164343.

Points to discuss IMO : 
- In the mochitest, forced to use a workaround in order to test the error case for the feature. I would like to keep the test, but if you have a cleaner way of doing this ...
- I realize I didn't think too much about the labeling. Currently using 'Copy data URI for image'. Maybe 'Copy Image as Data URI' would be more appropriate. 

Let me know what you think.

Comment 12

[Julian Descottes]

Attachment: bug965199-rebased.patch

(updated after rebase)

Comment 13

[Julian Descottes]

From the test :

> +  yield wait(3);

is a leftover that should be removed.

Comment 14

[(no longer active)]

(In reply to Julian Descottes from comment #10)
> From what I understood in nsClipboardHelper.cpp & nsTransferable.ccp a
> missing document means the transferable will assume the browser is not in
> private mode. However I don't see why this is needed for clipboard actions ?
> Can private browsing have an impact on the clipboard ?

We used to try to clear the contents of the clipboard when you closed your last private window if the contents were copied from a private window, so we used to need to know where content that ends up on the clipboard is coming from (as in, what document it's coming from, so that we could tell whether the document was private or not.)

We ended up removing this functionality in bug 815952, and right now, the originating document for a transferable is not used for anything, so we should just remove all of this code, I think.  We just haven't done that yet.

For this bug, you can just pass in null, or something, as the value doesn't end up being used anyway.

Comment 15

[(no longer active)]

(Note that the privacy state of a transferable is still used in other places, such as <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentAreaDragDrop.cpp#165>.)

Comment 16

[Julian Descottes]

@ehsan : thanks for clearing that up ! Will use a valid document for now. 

> privacy state of a transferable is still used in other places

Sure. Without removing the privacy state from the transferable, we could still remove the document from the clipboardHelper APIs. ClipboardHelper could then instantiate the transferable using a null document.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8606128 [details] [diff] [review]
bug965199-rebased.patch

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

Thanks Julian.
Some general feedback:
- it works \o/,
- the main code changes look really good, I only have a remark about avoiding to try/catch the whole method to let unrelated exceptions bubble through,
- the menu item label should read "Copy Image Data-URL" for consistency with the markup-view,
- my comments in rule-view.js apply to computed-view.js too,
- the test fails locally for me:

43 INFO TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_styleinspector_context-menu-copy-data-uri.js | Copy data URI is visible - 
Stack trace:
    chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_styleinspector_context-menu-copy-data-uri.js:testCopyDataUriToClipboard:75

And indeed, when that happens, the contextual menu doesn't have the item visible. This happens when testing the rule-view, with the invalid URL.
<span.ruleview-propertyvaluecontainer> gets passed to getNodeInfo, not the expected <a.theme-link>.
Can you investigate why?

::: browser/devtools/styleinspector/rule-view.js
@@ +1460,5 @@
>    /**
> +   * Check if the context menu popup was opened with a click on an image link
> +   * If true, save the image url to this._imageUrlToCopy
> +   */
> +  _isImageUrlPopup : function () {

really nitpicking: we usually format method declarations like:

name: function() {
not
name : function () {

@@ +1475,5 @@
> +
> +  /**
> +   * Check if a node is an image url
> +   * @param {DOMNode} node The node which we want information about
> +   * @return {Boolean} true if the node is an image url

Not strictly a Boolean return value because if nodeInfo is null (which it is, very often), then the function returns null.

Maybe rewrite:

_isImageUrlNode: function(node) {
  if (!node) {
    return false;
  }

  let nodeInfo = this.getNodeInfo(node);
  if (!nodeInfo) {
    return false;
  }

  return nodeInfo.type === overlays.VIEW_NODE_IMAGE_URL_TYPE;
}

@@ +1581,5 @@
>    /**
> +   * Retrieve the image data for the selected image url and copy it to the clipboard
> +   */
> +  _onCopyDataUri: Task.async(function*() {
> +    try {

It would be nice not to catch all possible exceptions here but only the one we know can happen and is fine when it does. Could you rewrite to something like:

_onCopyDataUri: Task.async(function*() {
  let inspectorFront = this.inspector.inspector;
  let text = _strings.GetStringFromName("styleinspector.contextmenu.copyDataUriError");

  try {
    let {data} = yield inspectorFront.getImageDataURL(this._imageUrlToCopy);
    text = yield data.string();
  } catch (e) {
    // getImageDataURL may fail if the URL isn't a valid image and that's fine,
    // the text placed in the clipboard will reflect this.
    if (typeof e !== "string" || !e.contains("Protocol error")) {
      throw e;
    }
  }

  clipboardHelper.copyString(text, this.doc);
}),

::: browser/devtools/styleinspector/test/browser_styleinspector_context-menu-copy-data-uri.js
@@ +26,5 @@
> +    '<div class="invalid-background">Invalid background image</div>'
> +  ].join("\n");
> +
> +  yield addTab("data:text/html;charset=utf8,Test context menu Copy Data URI");
> +  content.document.body.innerHTML = PAGE_CONTENT;

Can you replace those 2 lines with:

yield addTab("data:text/html;charset=utf8," + encodeURIComponent(PAGE_CONTENT));

This avoid having to manipulate the document HTML through a CPOW when the test is run in e10s mode, sine we're trying to get rid of CPOW usage
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers

@@ +52,5 @@
> +
> +function* testCopyDataUriToClipboard({view, inspector}, selector, expected) {
> +  info("Select node in inspector panel");
> +  yield selectNode(selector, inspector);
> +  ok(true, "Node is selected");

I don't think this assertion is needed.

@@ +61,5 @@
> +  ok(imageLink, "Background-image link element found");
> +
> +  info("Simulate right click on the background image URL");
> +  let popup = once(view._contextmenu, "popupshown");
> +  // Cannot rely on synthesizeMouseAtCenter here. The data-uri is displayed on 2 lines.

You probably meant to say "The image url may be displayed on 2 lines."
Anyway, good call on this, and thanks for adding that comment.

@@ +70,5 @@
> +  EventUtils.synthesizeMouse(imageLink, x, y, {button: 2, type: "contextmenu"}, getViewWindow(view));
> +  yield popup;
> +  ok(true, "Context menu is displayed");
> +
> +  info('Check that Copy Data URI menu entry is displayed');

Can you double-quote all strings to be consistent throughout the file?

@@ +74,5 @@
> +  info('Check that Copy Data URI menu entry is displayed');
> +  ok(!view.menuitemCopyDataUri.hidden, "Copy data URI is visible");
> +
> +  info('Click Copy Data URI and wait for clipboard');
> +  yield wait(3);

As you said, this should be removed.

@@ +82,5 @@
> +  view._contextmenu.hidePopup();
> +}
> +
> +function getBackgroundImageProperty(view, selector) {
> +  let isRuleView = !!view.doc;

Or, perhaps more future proof:

let isRuleView = view instanceof CssRuleView;

@@ +89,5 @@
> +  } else {
> +    return getComputedViewProperty(view, "background-image");
> +  }
> +}
> +/**

nit: missing empty line between those 2 functions.

@@ +93,5 @@
> +/**
> + * Function that returns the window for a given view.
> + */
> +function getViewWindow(view) {
> +  let viewDocument = view.styleDocument ? view.styleDocument : view.doc;

Aligning the way these 2 views reference their documents could be a good candidate to add to the refactoring bug you created.

@@ +95,5 @@
> + */
> +function getViewWindow(view) {
> +  let viewDocument = view.styleDocument ? view.styleDocument : view.doc;
> +  return viewDocument.defaultView;
> +}
\ No newline at end of file

nit: Missing newline at the end of the file.

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +96,5 @@
>  # the rule and computed view context menu "Copy Color" entry.
>  ruleView.contextmenu.copyColor.accessKey=L
>  
> +# LOCALIZATION NOTE (ruleView.contextmenu.copyDataUri): Text displayed in the rule
> +# and computed view context menu when an image link was clicked.

Can you elaborate a little, the more details the better:

"Text displayed in an item of the rule and computed view context menu when an image link was clicked. Clicking the item copies the data from the image to the clipboard".

@@ +100,5 @@
> +# and computed view context menu when an image link was clicked.
> +styleinspector.contextmenu.copyDataUri=Copy data URI for image
> +
> +# LOCALIZATION NOTE (ruleView.contextmenu.copyDataUri): Text set in the clipboard
> +# when "Copy Image Data URI" fails to retrieve the image data

"[...] which can happen if the clicked link does not point to an image"

Comment 18

[Julian Descottes]

@Patrick : Thanks for the review ! I'll try to submit a new patch later today. 

About the test, I ran it last week on both Win 7 and Linux without issues. Either due to recent code changes or to Mac OS X.

Regarding the styling issues, is there a styleguide I can use to setup a linting/styling tool ?

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes from comment #18)
> Regarding the styling issues, is there a styleguide I can use to setup a
> linting/styling tool ?
Unfortunately we don't have a styleguide that I'm aware of right now. But we should totally do, to avoid loosing time on minor corrections that do not bring value to the patch.
The Loop team (working on Firefox Hello) started introducing linting using eslint and I've been playing with that for the last couple of days. I'll probably send an email to the devtools mailing list to see if we can get something started for devtools. In the meantime, if you have SublimeText3 installed + SublimeLinter and the eslint-contrib addon, feel free to drop this .eslintrc [1] file in /browser/devtools to have the same configuration that I began using. It isn't finalized, but it's close enough to the style we normally use in the codebase.

[1] https://dl.dropboxusercontent.com/u/714210/eslintrc

Comment 20

[Julian Descottes]

> if (typeof e !== "string" || !e.contains("Protocol error")) {

@Patrick : Instead of doing this test, I think we could throw all errors and simply use the catch in order to fill the user's clipboard with the error message. This way, no error gets swallowed and the user still gets some feedback. Something like : 

>  _onCopyDataUri: Task.async(function*() {
>    let message;
>    try {
>      let inspectorFront = this.inspector.inspector;
>      let data = yield inspectorFront.getImageDataFromURL(this._imageUrlToCopy);
>      message = yield data.data.string();
>    } catch (e) {
>      message = _strings.GetStringFromName("styleinspector.contextmenu.copyDataUriError");
>      throw(e);
>    } finally {
>      clipboardHelper.copyString(message, this.doc);
>    }
>  }),

(I could get rid of the catch here and simply do a try finally, but I think the intent would become harder to grasp)

Anyway for reference the alternative would be 

>  _onCopyDataUri: Task.async(function*() {
>    let message = _strings.GetStringFromName("styleinspector.contextmenu.copyDataUriError");
>    try {
>      let inspectorFront = this.inspector.inspector;
>      let data = yield inspectorFront.getImageDataFromURL(this._imageUrlToCopy);
>      message = yield data.data.string();
>    } finally {
>      clipboardHelper.copyString(message, this.doc);
>    }
>  }),

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes from comment #20)
> > if (typeof e !== "string" || !e.contains("Protocol error")) {
> 
> @Patrick : Instead of doing this test, I think we could throw all errors and
> simply use the catch in order to fill the user's clipboard with the error
> message. This way, no error gets swallowed and the user still gets some
> feedback. Something like : 
> 
> >  _onCopyDataUri: Task.async(function*() {
> >    let message;
> >    try {
> >      let inspectorFront = this.inspector.inspector;
> >      let data = yield inspectorFront.getImageDataFromURL(this._imageUrlToCopy);
> >      message = yield data.data.string();
> >    } catch (e) {
> >      message = _strings.GetStringFromName("styleinspector.contextmenu.copyDataUriError");
> >      throw(e);
> >    } finally {
> >      clipboardHelper.copyString(message, this.doc);
> >    }
> >  }),
Sounds good to me.

Comment 22

[Julian Descottes]

Attachment: bug965199.patch

@Patrick : Applied the suggestions from your review. For consistency, I also tried to stick to 'Copy Image Data-URL' everywhere. For example now my menu property is called menuitemCopyImageDataUrl instead of menuitemCopyDataUri.

For the failing test, I now calculate the coordinates of the center of the first clientRect of the <a> element. I did not setup a MacOS environment yet, but maybe this could fix the issue you had ? If you can give it a try it would be great !

Thanks

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8607946 [details] [diff] [review]
bug965199.patch

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

Those changes look good, thanks for addressing all my previous comments.
r=me with a green try build.
Talking about this, the test still fails for me locally, but not at the same place (at least I don't think so).
I'll spend some time now to try and understand why. Will post a message back here when I do.

Comment 24

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> Talking about this, the test still fails for me locally, but not at the same
> place (at least I don't think so).
So, the test fails for me locally because the click event is simulated outside the firefox window.
When the test opens the computed view, the size of the style-inspector sidebar is such that the image link is far to the right and has an ellipsis. When the test gets the coordinates of the rect and finds its center, it finds a point that's outside the window, so simulating the click fails.
If instead of clicking at the center we click at 2px from the top left corner, everything is fine.

Comment 25

[Patrick Brosset <:pbro>]

In practice, this works for me:

  let rect = imageLink.getClientRects()[0];
  let x = rect.left + rect.width/2;
  let y = rect.top + rect.height/2;

this doesn't:

  let rect = imageLink.getClientRects()[0];
  let x = rect.left + 2;
  let y = rect.top + 2;

Comment 26

[Julian Descottes]

Attachment: bug965199_copy_image_data_url.diff

@Patrick : Thanks for the hint, modified the test with your suggestion.

Comment 27

[(no longer active)]

(In reply to Julian Descottes from comment #16)
> @ehsan : thanks for clearing that up ! Will use a valid document for now. 
> 
> > privacy state of a transferable is still used in other places
> 
> Sure. Without removing the privacy state from the transferable, we could
> still remove the document from the clipboardHelper APIs. ClipboardHelper
> could then instantiate the transferable using a null document.

Yeah, that seems like the right thing to do.  Filed bug 1166840.

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8608238 [details] [diff] [review]
bug965199_copy_image_data_url.diff

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

Cool, good to go now.
Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2af6d6bd31e

Comment 29

[Julian Descottes]

As discussed, tests failed due to "A promise chain failed to handle a rejection". This is coming from the error thrown in the catch statement of onCopyImageDataUrl in rule-view and computed view. 

From the test, this is triggered by 

> view.menuitemCopyImageDataUrl.click()

For now I don't see how I could handle errors triggered from a direct call to click(), but I will investigate. 

By the way, I realized rule-view and computed-view were not using the same code for onCopyImageDataUrl, which I need to correct.

Comment 30

[Julian Descottes]

Attachment: bug965199_copy_image_data_url_2.diff

After discussing with Patrick, decided to remove the throw(e) from the try catch in _onCopyImageDataUrl. Errors are still logged by the server if the call to getImageDataFromUrl fails.

The test is OK now (still no idea how I missed the failures locally :( ).

The only changes from the previous patch here are about _onCopyImageDataUrl in both computed-view.js and rule-view.js :

> _onCopyImageDataUrl: Task.async(function*() {
>   let message;
>   try {
>     let inspectorFront = this.inspector.inspector;
>     let data = yield inspectorFront.getImageDataFromURL(this._imageUrlToCopy);
>     message = yield data.data.string();
>   } catch (e) {
>     message = CssHtmlTree.l10n("styleinspector.copyImageDataUrlError");
>   }
> 
>   clipboardHelper.copyString(message, this.styleDocument);
> })

@Patrick : Can you review and push to try if it's ok ?

Comment 31

[Patrick Brosset <:pbro>]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95e4f967fdc
(less jobs, we know the rest is green).

Comment 32

[Patrick Brosset <:pbro>]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=366658372763

Comment 33

[Julian Descottes]

Attachment: bug965199_copy_image_data_url_3.diff

Rebased with the changes from Bug 1166840. Removed useless document argument from rule view and computed view.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6c3041c20b6f

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/6c3041c20b6f

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c3041c20b6f

Comment 37

[Saheda Reza [:Antora]]

I didn't found "Copy as data URI" on CSS panels on Firefox Nightly:

Build ID	 20140129030602
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0

It's found on Firefox Developer Edition

Build ID 	20150731004008
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:41.0) Gecko/20100101 Firefox/41.0

[buday-20150729]

Comment 38

[Julian Descottes]

@Antora :

> I didn't found "Copy as data URI" on CSS panels on Firefox Nightly:

> Build ID	 20140129030602
> User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0

According to this, your Nightly is a Firefox 29 ? This feature was released in Firefox 41, and I just checked on a freshly built nightly, everything seems fine.

Comment 39

[Saheda Reza [:Antora]]

That's what I said, I just verified the implementation of this feature on Firefox 41 :)

Comment 40

[Julian Descottes]

Oh ok sorry, misunderstood your message :)

Comment 41

[Bogdan Maris, Desktop QA]

Thank you, Antora!

Marking as verified fixed based on your testing.