Closed
Bug 1063463
Opened 10 years ago
Closed 10 years ago
Follow-up fix for editing mixed-case attributes in the inspector
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(1 file)
5.10 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
This is a follow-up to bug 1008584 which fixed the issue, but with a less than ideal solution.
As explained in bug 1008584 comment 17, we can keep simply parsing with the HTML parser but by defining the parsed tag to be <svg> the attributes case will be preserved.
Assignee | ||
Comment 1•10 years ago
|
||
This should work better, and is simpler, than the previous solution.
Pending try: https://tbpl.mozilla.org/?tree=Try&rev=d2a50de15215
Comment 2•10 years ago
|
||
Comment on attachment 8485764 [details] [diff] [review]
bug1063463-preserve-case-when-editing-attributes v1.patch
Review of attachment 8485764 [details] [diff] [review]:
-----------------------------------------------------------------
I do wonder if there is some weird case that setting an attribute on an SVG element would give different behavior than a div, but if the test harness still works I think this is a nice, simpler solution.
One question: I've applied the patch and add viewBox="foo", the markup view seems to still lower case the attribute name. Are you seeing this with the patch applied?
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8485764 [details] [diff] [review]
> bug1063463-preserve-case-when-editing-attributes v1.patch
>
> Review of attachment 8485764 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I do wonder if there is some weird case that setting an attribute on an SVG
> element would give different behavior than a div, but if the test harness
> still works I think this is a nice, simpler solution.
Yeah, I think our test suite covers the basics well, so we should be fine, we'll keep our eyes open for potential follow-ups.
> One question: I've applied the patch and add viewBox="foo", the markup view
> seems to still lower case the attribute name. Are you seeing this with the
> patch applied?
Weird, I'm not seeing this. The new test passes fine, and I've tested locally, adding/editing a mixed-case attributes, that seemed to have worked.
I didn't test specifically with 'viewBox="foo"' though, maybe there's a problem with the parser validating the value of the SVG attribute. I'll test this as soon as I've got a chance.
Assignee | ||
Comment 4•10 years ago
|
||
- Adding attribute viewBox="foo" on a <p> element produces viewbox="foo". The parser in markup-view.js does parse name=viewBox/value=foo, but once the server, the attribute is added to the node via rawNode.setAttribute(name, value) and that lowercases it. This is expected I think, since attributes are case insensitive.
- Adding attribute viewBox="foo" on a <svg> element produces viewBox="foo", which is what we wanted to fix in the first place, so that's fine.
- Adding attribute viewBox="0 0 1 1" on a <svg> element produces viewBox="0 0 1 1". Fine too.
- Adding attribute aA="a" on any element produces aa="a", even on <svg> elements. This is because the HTML parser (in markup-view.js) probably checks if the attribute is a valid <svg> attribute before preserving the case. This is also probably a good thing right? There's no use preserving random mixed-case attributes.
Note that even if we use <svg> as the tag to parse, even valid svg attributes that are normally used on other tagnames (like attributeName which is used on <animate>) do work as expected.
Comment 5•10 years ago
|
||
Comment on attachment 8485764 [details] [diff] [review]
bug1063463-preserve-case-when-editing-attributes v1.patch
Review of attachment 8485764 [details] [diff] [review]:
-----------------------------------------------------------------
> - Adding attribute viewBox="foo" on a <p> element produces viewbox="foo".
> The parser in markup-view.js does parse name=viewBox/value=foo, but once the
> server, the attribute is added to the node via rawNode.setAttribute(name,
> value) and that lowercases it. This is expected I think, since attributes
> are case insensitive.
Makes sense
Attachment #8485764 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•