Closed
Bug 1133205
Opened 9 years ago
Closed 9 years ago
Editing SVG styles using rule view fails
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
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)
4.57 KB,
patch
|
ntim
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Blocks: svg-devtools
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Simple but effective fix. Patrick, do we need tests for this ?
Attachment #8566247 -
Flags: review?(pbrosset)
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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•9 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
Comment 5•9 years ago
|
||
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•9 years ago
|
||
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 7•9 years ago
|
||
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•9 years ago
|
||
Thanks for the review ! Updated the comment.
Attachment #8568025 -
Attachment is obsolete: true
Attachment #8568115 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/85f3561e6c82
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•