Closed Bug 1222586 Opened 4 years ago Closed 3 years ago

"Paste as first/last child" context menu doesn't work on SVG files

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox51 fixed)

VERIFIED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: ntim, Assigned: fayolle-florent)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 file, 6 obsolete files)

TypeError: wc is undefined: trackSelection/</</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://react-devtools/main/trackSelection.js:39:9
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
Selection.prototype.setNodeFront@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/selection.js:179:5
MarkupView.prototype.markNodeAsSelected@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:1285:7
MarkupView.prototype.navigate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:760:5
MarkupContainer.prototype._onMouseDown@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:1895:5
EventListener.handleEvent*MarkupContainer.prototype.initialize@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:1698:5
MarkupElementContainer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:2179:3
MarkupView.prototype.importNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:792:19
MarkupView.prototype.showNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:945:5
MarkupView.prototype._onNewSelection@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/markupview/markup-view.js:472:14
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
Selection.prototype.setNodeFront@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/selection.js:179:5
InspectorPanel.prototype._deferredOpen/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:204:7
EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:92:1
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
InspectorPanel.prototype._onMarkupFrameLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:937:5
EventListener.handleEvent*InspectorPanel.prototype._initMarkup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:918:5
InspectorPanel.prototype._deferredOpen@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:197:5
InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:99:14
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:275:14
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.WalkerFront<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3287:12
InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:266:14
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
exports.InspectorFront<.getPageStyle<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3789:12
InspectorPanel.prototype._getPageStyle@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:227:12
InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:95:14
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:451:5
InspectorPanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:94:12
Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1236:19
 event-emitter.js:152:0
Error writing response to: getUniqueSelector protocol.js:1025
<unavailable> protocol.js:907
Protocol error (unknownError): Error: undefined passed where a value is required inspector-panel.js:465
<unavailable> protocol.js:907
STR:
Go to a SVG file (testcase url)
Copy the <path> as outer html
Try to paste it as last child of the <svg> root element
Blocks: svg-devtools
Whiteboard: [polish-backlog]
Triaging (filter on CLIMBING SHOES).
Priority: -- → P3
Whiteboard: [polish-backlog] → [btpp-backlog]
See Also: → 1297633
Test case:
1. Open the Inspector on https://mdn.mozillademos.org/files/12668/MDN.svg
2. Right-click the <g id="g5461"> element and choose Copy > Outer HTML from the context menu
3. Right-click the <svg> tag and choose Paste > As Last Child from the context menu

=> The copied SVG doesn't get inserted and instead an error message is thrown.

With enabled e10s I only get this as error message:

<unavailable>   protocol.js:906

With disabled e10s I get this:

Error: Can't insert adjacent element to the root.
Stack trace:
WalkerActor<.insertAdjacentHTML@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:1993:13
generateRequestHandlers/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1042:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1748:15
ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7

The related condition is here:
https://hg.mozilla.org/mozilla-central/file/e9533997fcef/devtools/server/actors/inspector.js#l1994

Florent, that looks like your code, so I though maybe you could have a look at it.

Sebastian
Flags: needinfo?(fayolle-florent)
Oh, I see. The comment in the code says:
>    // Don't insert anything adjacent to the document element,
>    // the head or the body.
>    if (node.isDocumentElement()) {
>      throw new Error("Can't insert adjacent element to the root.");
>    }

This condition is relevant only for HTML documents, not for XML ones.

Two solutions :
- The simple one: don't raise exception if the user inserts a sibling of the HEAD or BODY elements.
- Or: raise an exception if the user inserts a sibling of the HEAD/BODY (as currently implemented), but support the insertion of elements as first child of the root of any XML documents (so support special cases when the document is an XML and non-XHTML one)

The first option seems to me the simplest, safest and the most compliant to how Element.prototype.insertAdjacentHTML behaves.

Thoughts? I'll take a look soon.

Florent
I'd also prefer the first solution. Note that the options to paste the copied structure are deactivated anyway, so this issue does not occur on HTML, anyway. And if people really want to add elements other than <head> and <body> to <html> is up to them (and already possible via drag & drop), IMO.

(In reply to Florent Fayolle from comment #4)
> Thoughts? I'll take a look soon.

If you want to take it, you can remove the need-info flag.

Sebastian
Flags: needinfo?(fayolle-florent)
I would like to take the bug, but I cannot assign it to myself. Thanks :)

Florent
(In reply to Florent Fayolle from comment #6)
> I would like to take the bug, but I cannot assign it to myself. Thanks :)

Did it for you. :-)

Sebastian
Assignee: nobody → fayolle-florent
Attached patch 1222586.patch (obsolete) — Splinter Review
Thanks Sebastian!

:pbrosset is it useful to add a test for this?

Florent
Attachment #8785680 - Flags: review?(pbrosset)
(In reply to Florent Fayolle from comment #8)
> Created attachment 8785680 [details] [diff] [review]
> 1222586.patch
> 
> Thanks Sebastian!
> 
> :pbrosset is it useful to add a test for this?
Yes, it would be nice, so we never break this again in the future.
Comment on attachment 8785680 [details] [diff] [review]
1222586.patch

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

This seems sane to me. But, yes, please do add a test for this to avoid future regressions.
Attachment #8785680 - Flags: review?(pbrosset) → feedback+
Attached patch 1222586.patch (obsolete) — Splinter Review
OK, here is the updated patch.

I took a name for the test file similar to the one for pasting HTML content (browser_inspector_menu-03-paste-items-svg.js VS browser_inspector_menu-03-paste-items.js). Is it OK for you?

Also I wonder if the test shouldn't rather be a unit test with mocking rather than testing the whole UI, as the test targets the behavior of a specific function. Are there stuffs in place that allows this?

Florent
Attachment #8785680 - Attachment is obsolete: true
Attachment #8787974 - Flags: review?(pbrosset)
The patch is missing doc_svg_inspector_menu.svg
Attached patch 1222586.patch (obsolete) — Splinter Review
Thanks Tim!

Florent
Attachment #8787974 - Attachment is obsolete: true
Attachment #8787974 - Flags: review?(pbrosset)
Attachment #8787991 - Flags: review?(pbrosset)
Comment on attachment 8787991 [details] [diff] [review]
1222586.patch

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

Just a few final adjustments in the test and we're good to go!

::: devtools/client/inspector/test/browser.ini
@@ +33,5 @@
>    doc_inspector_search-suggestions.html
>    doc_inspector_search-svg.html
>    doc_inspector_select-last-selected-01.html
>    doc_inspector_select-last-selected-02.html
> +  doc_svg_inspector_menu.svg

Please rename such that it has the same prefix as all other support files in this folder:
doc_inspector_svg.svg for instance.

::: devtools/client/inspector/test/browser_inspector_menu-03-paste-items-svg.js
@@ +22,5 @@
> +var clipboard = require("sdk/clipboard");
> +registerCleanupFunction(() => {
> +  clipboard = null;
> +});
> +

nit: there are 2 empty new lines here. We only allow 1 in our eslint rules.

@@ +29,5 @@
> +  let { inspector, testActor } = yield openInspectorForURL(TEST_URL);
> +
> +  yield testPasteAdjacentHTMLMenu();
> +
> +  function* testPasteAdjacentHTMLMenu() {

Why use an extra nested function here? You could just put the whole body of the function inside the add_task body and avoid 1 level of indentation.

@@ +33,5 @@
> +  function* testPasteAdjacentHTMLMenu() {
> +    let refSelector = "svg";
> +    let oldHTML = yield testActor.getProperty(refSelector, "innerHTML");
> +    let nodeFront = yield getNodeFront(refSelector, inspector);
> +    yield selectNode(nodeFront, inspector);

selectNode accepts both nodeFronts and selectors, so you can save yourself the trouble of calling getNodeFront first. Also you can use getContainerForSelector instead of getContainerForNodeFront below.

@@ +36,5 @@
> +    let nodeFront = yield getNodeFront(refSelector, inspector);
> +    yield selectNode(nodeFront, inspector);
> +    let markupTagLine = getContainerForNodeFront(nodeFront, inspector).tagLine;
> +
> +    for (let { clipboardData, menuId } of PASTE_ADJACENT_HTML_DATA) {

You're not defining 'desc' and don't use it in the for loop. You probably just missed it.

@@ +51,5 @@
> +    }
> +
> +    let html = yield testActor.getProperty(refSelector, "innerHTML");
> +    let expectedHtml = PASTE_ADJACENT_HTML_DATA[0].clipboardData +
> +        oldHTML + PASTE_ADJACENT_HTML_DATA[1].clipboardData;

It feels a bit weird using a test array and then accessing items with index 0 and 1 at the end. Let me explain:
If you have a test array at the start of the test, it tells people that these are test cases, and that it's simple to add more test cases by adding things into the array later, if we need it in the future.
But here, you're relying on the 2 first items only to test something. So really, you shouldn't use an array I think, and instead just 2 const:

const PASTE_AS_FIRST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg" cx="42" cy="42" r="5"/>';
const PASTE_AS_LAST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg" cx="42" cy="42" r="15"/>';

And then remove the for loop, just paste the 2 strings one by one, and finally test the expected html is:

PASTE_AS_FIRST_CHILD + oldHTML + PASTE_AS_LAST_CHILD
Attachment #8787991 - Flags: review?(pbrosset) → feedback+
Comment on attachment 8787991 [details] [diff] [review]
1222586.patch

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

::: devtools/client/inspector/test/doc_svg_inspector_menu.svg
@@ +7,5 @@
> +   xmlns="http://www.w3.org/2000/svg"
> +   id="svg3624"
> +   height="200"
> +   width="226.85512"
> +   version="1.1">

Please run this SVG through SVGO: https://jakearchibald.github.io/svgomg/
Make sure the Prettify code option is on.
Attached patch 1222586.patch (obsolete) — Splinter Review
(In reply to Patrick Brosset <:pbro> from comment #14)
> Comment on attachment 8787991 [details] [diff] [review]
> 1222586.patch
> 
> Review of attachment 8787991 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> @@ +29,5 @@
> > +  let { inspector, testActor } = yield openInspectorForURL(TEST_URL);
> > +
> > +  yield testPasteAdjacentHTMLMenu();
> > +
> > +  function* testPasteAdjacentHTMLMenu() {
> 
> Why use an extra nested function here? You could just put the whole body of
> the function inside the add_task body and avoid 1 level of indentation.


Very true! Removed.

> 
> @@ +33,5 @@
> > +  function* testPasteAdjacentHTMLMenu() {
> > +    let refSelector = "svg";
> > +    let oldHTML = yield testActor.getProperty(refSelector, "innerHTML");
> > +    let nodeFront = yield getNodeFront(refSelector, inspector);
> > +    yield selectNode(nodeFront, inspector);
> 
> selectNode accepts both nodeFronts and selectors, so you can save yourself
> the trouble of calling getNodeFront first. Also you can use
> getContainerForSelector instead of getContainerForNodeFront below.

Done.


> 
> @@ +51,5 @@
> > +    }
> > +
> > +    let html = yield testActor.getProperty(refSelector, "innerHTML");
> > +    let expectedHtml = PASTE_ADJACENT_HTML_DATA[0].clipboardData +
> > +        oldHTML + PASTE_ADJACENT_HTML_DATA[1].clipboardData;
> 
> It feels a bit weird using a test array and then accessing items with index
> 0 and 1 at the end. Let me explain:
> If you have a test array at the start of the test, it tells people that
> these are test cases, and that it's simple to add more test cases by adding
> things into the array later, if we need it in the future.
> But here, you're relying on the 2 first items only to test something. So
> really, you shouldn't use an array I think, and instead just 2 const:
> 
> const PASTE_AS_FIRST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg"
> cx="42" cy="42" r="5"/>';
> const PASTE_AS_LAST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg"
> cx="42" cy="42" r="15"/>';
> 
> And then remove the for loop, just paste the 2 strings one by one, and
> finally test the expected html is:
> 
> PASTE_AS_FIRST_CHILD + oldHTML + PASTE_AS_LAST_CHILD

Oh yeah very good point, thanks!

> Please run this SVG through SVGO: https://jakearchibald.github.io/svgomg/
> Make sure the Prettify code option is on.

Oh yes, there was a lot of noise in this file. That's done, thanks, although I get some obscure exception now:

> console.error:
>   Error writing response to: getUniqueSelector
> console.error:
>   Message: Error: undefined passed where a value is required
>   Stack:
>     identityWrite@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools
> /shared/protocol.js:109:11
> RetVal<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/sha
> red/protocol.js:528:12
> Response<.write/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools
> /shared/protocol.js:681:16
> …

Not sure whether that's bad or not, as it doesn't prevent the test to pass nor to fail. What do you think?

Florent
Attachment #8788604 - Flags: review?(pbrosset)
Attachment #8788604 - Flags: review?(ntim.bugs)
Comment on attachment 8788604 [details] [diff] [review]
1222586.patch

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

You forgot to `hg add` SVG file :) I recommend using `hg rename` when you rename files btw.

::: devtools/client/inspector/test/browser_inspector_menu-03-paste-items-svg.js
@@ +7,5 @@
> +
> +const TEST_URL = URL_ROOT + "doc_inspector_svg.svg";
> +const PASTE_AS_FIRST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg" cx="42" cy="42" r="5"/>';
> +const PASTE_AS_LAST_CHILD = '<circle xmlns="http://www.w3.org/2000/svg" cx="42" cy="42" r="15"/>';
> +const PASTE_ADJACENT_HTML_DATA = [

Looks unused, please remove this (Eslint usually catches this btw).

@@ +20,5 @@
> +];
> +
> +var clipboard = require("sdk/clipboard");
> +registerCleanupFunction(() => {
> +  clipboard = null;

So instead of doing this, I recommend making clipboard a local `let` variable inside the body of add_task();
Attachment #8788604 - Flags: review?(ntim.bugs) → feedback+
Attached patch 1222586.patch (obsolete) — Splinter Review
Many thanks Tim for the tips! Here is the patch updated.

Florent
Attachment #8787991 - Attachment is obsolete: true
Attachment #8788604 - Attachment is obsolete: true
Attachment #8788604 - Flags: review?(pbrosset)
Attachment #8788758 - Flags: review?(pbrosset)
Attachment #8788758 - Flags: review?(ntim.bugs)
Comment on attachment 8788758 [details] [diff] [review]
1222586.patch

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

Looks good to me!

::: devtools/client/inspector/test/browser_inspector_menu-03-paste-items-svg.js
@@ +26,5 @@
> +  yield pasteContent("node-menu-pastelastchild", PASTE_AS_LAST_CHILD);
> +
> +  let html = yield testActor.getProperty(refSelector, "innerHTML");
> +  let expectedHtml = PASTE_AS_FIRST_CHILD + oldHTML + PASTE_AS_LAST_CHILD;
> +  ok(html === expectedHtml);

Please use `is` instead of `ok` in these cases:
is(html, expectedHtml, "The new innerHTMLof the SVG node is correct");
Attachment #8788758 - Flags: review?(pbrosset) → review+
Comment on attachment 8788758 [details] [diff] [review]
1222586.patch

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

Agreed with pbro on using is instead of ok. Make sure you push this to try.

::: devtools/client/inspector/test/browser_inspector_menu-03-paste-items-svg.js
@@ +11,5 @@
> +
> +add_task(function* () {
> +  let clipboard = require("sdk/clipboard");
> +  registerCleanupFunction(() => {
> +    clipboard = null;

You should be able to remove the cleanup function. The clipboard var is only available within the body of add_task, so it will get cleaned up automatically anyway.

::: devtools/client/inspector/test/doc_inspector_svg.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

Can you remove this line?
Attachment #8788758 - Flags: review?(ntim.bugs) → review+
Attached patch 1222586.patch (obsolete) — Splinter Review
I took your remarks into account. 

Could you try-push for me? My account has been disabled since my last contribution (if I resume contributing more, I will ask for a reactivation).

Thanks!

Florent
Attachment #8788758 - Attachment is obsolete: true
Attachment #8789039 - Flags: review?(pbrosset)
Attachment #8789039 - Flags: review?(ntim.bugs)
Comment on attachment 8789039 [details] [diff] [review]
1222586.patch

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

Thanks. I gave this one last test and it worked fine.
Just one comment about the test, but no need to ask for another review after this.
Also, another comment about the commit message: it should not just state the bug title, it should explain what you actually fixed.
Maybe something like Bug 1222586 - Don't assume document to be HTML when pasting markup into the markup-view; r=pbro
Or something that you feel makes more sense.

I've pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cba8f0fb769

::: devtools/client/inspector/test/browser_inspector_menu-03-paste-items-svg.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Test that different paste items work in the context menu

nit: please update this comment. It should say that this tests that HTML can be pasted in SVG elements.
Attachment #8789039 - Flags: review?(pbrosset) → review+
Comment on attachment 8789039 [details] [diff] [review]
1222586.patch

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

pbro's r+ should be enough :) Try is green, so feel free to mark checkin-needed once you've addressed Patrick's comment.
Attachment #8789039 - Flags: review?(ntim.bugs)
Attached patch 1222586.patchSplinter Review
Thank you very much Patrick, Tim!

@Patrick I took into account your last comments. I mark the bug as checkin-needed as Tim suggests.

Florent
Attachment #8789039 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/cd2f33114ac4
Don't assume document to be HTML when pasting markup into the markup-view; r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd2f33114ac4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Works fine for me. Merci beaucoup, Florent ! :-)

Sebastian
Status: RESOLVED → VERIFIED
Bitte schöne! =)

Florent
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.