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)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file)

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.
See Also: → 1008584
This should work better, and is simpler, than the previous solution. Pending try: https://tbpl.mozilla.org/?tree=Try&rev=d2a50de15215
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8485764 - Flags: review?(bgrinstead)
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?
(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.
- 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 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+
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: