Editing SVG styles using rule view fails

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1133682
(Assignee)

Updated

3 years ago
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 1134412
(Assignee)

Comment 1

3 years ago
Created attachment 8566247 [details] [diff] [review]
Patch

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.
(Assignee)

Comment 4

3 years ago
(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");
(Assignee)

Comment 6

3 years ago
Created attachment 8568025 [details] [diff] [review]
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
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+
(Assignee)

Comment 8

3 years ago
Created attachment 8568115 [details] [diff] [review]
Patch v2.1

Thanks for the review ! Updated the comment.
Attachment #8568025 - Attachment is obsolete: true
Attachment #8568115 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85f3561e6c82
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.