Closed Bug 1133205 Opened 9 years ago Closed 9 years ago

Editing SVG styles using rule view fails

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Blocks: svg-devtools
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Blocks: 1134412
Attached patch Patch (obsolete) — Splinter Review
Simple but effective fix.

Patrick, do we need tests for this ?
Attachment #8566247 - Flags: review?(pbrosset)
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.
Attachment #8566247 - Flags: review?(pbrosset)
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.
(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
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");
Attached patch Patch v2 (obsolete) — Splinter Review
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
Attachment #8566247 - Attachment is obsolete: true
Attachment #8568025 - Flags: review?(pbrosset)
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.
Attachment #8568025 - Flags: review?(pbrosset) → review+
Attached patch Patch v2.1Splinter Review
Thanks for the review ! Updated the comment.
Attachment #8568025 - Attachment is obsolete: true
Attachment #8568115 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85f3561e6c82
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.