1133205 - Editing SVG styles using rule view fails

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

Simple but effective fix.

Patrick, do we need tests for this ?

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8566247 [details] [diff] [review]
Patch

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

::: toolkit/devtools/server/actors/styles.js
@@ +1067,5 @@
>  
>        document = this.getDocument(parentStyleSheet);
>      }
>  
> +    let tempElement = document.createElementNS("http://www.w3.org/1999/xhtml", "div");

Please use the XHTML_NS const instead.

Comment 3

[Patrick Brosset <:pbro>]

Before R+'ing this, I'd like to know the steps the reproduce and what fails exactly.
I've quickly tested a few things that seemed to work, so obviously, I'm not testing the right thing.
About the test, I do think we need one :)
But let's see those STR first.

Comment 4

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> Before R+'ing this, I'd like to know the steps the reproduce and what fails
> exactly.
> I've quickly tested a few things that seemed to work, so obviously, I'm not
> testing the right thing.
> About the test, I do think we need one :)
> But let's see those STR first.

STR :
- open the inspector on an SVG document
- inspect an element
- try to create a style, or edit an inline style
- non-inline styles may fail as well, but I haven't checked

AR :
- the style isn't updated (you can set the fill to red if you want an obvious indicator)
- you get this error into the browser console : tempElement.style is undefined (or null, I don't remember)

ER :
- the style should be updated

Comment 5

[Patrick Brosset <:pbro>]

Ah thanks! I was testing on SVG elements inside an HTML document.
I was able to reproduce the issue just now.
So, the patch looks fine, but I'd like a test added in /browser/devtools/styleinspector/test
It doesn't need to be complex, but it should at least:
- open a SVG document
- open the style-inspector
- try to add an inline style in the 'element' rule
- check that the style was applied correctly.

To check that a style change did work, you can do something like this:

is((yield getComputedStyleProperty(selector, null, "fill")), "red", "The fill was changed to red");

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch v2

Now includes a test (mainly copied from the browser_ruleview_add-property-*.js files). The test fully passes.

Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=23cdeb636635

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8568025 [details] [diff] [review]
Patch v2

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

That looks great. Could you just update that test comment?
No other remarks. Let's check this in as soon as try is green.
Thanks for fixing this!

::: browser/devtools/styleinspector/test/browser_ruleview_add-property-svg.js
@@ +5,5 @@
> +"use strict";
> +
> +// Testing various inplace-editor behaviors in the rule-view
> +// FIXME: To be split in several test files, and some of the inplace-editor
> +// focus/blur/commit/revert stuff should be factored out in head.js

Can you change this comment please? It needs to say what this test is about.

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Thanks for the review ! Updated the comment.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/85f3561e6c82