1063463 - Follow-up fix for editing mixed-case attributes in the inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1063463-preserve-case-when-editing-attributes v1.patch

This should work better, and is simpler, than the previous solution.
Pending try: https://tbpl.mozilla.org/?tree=Try&rev=d2a50de15215

Comment 2

[Brian Grinstead [:bgrins]]

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?

Comment 3

[Patrick Brosset <:pbro>]

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

Comment 4

[Patrick Brosset <:pbro>]

- 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

[Brian Grinstead [:bgrins]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2bd7bbea2d05

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd7bbea2d05