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

RESOLVED FIXED in Firefox 35

Status

RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 35
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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)

Updated

4 years ago
See Also: → bug 1008584
(Assignee)

Comment 1

4 years ago
Created attachment 8485764 [details] [diff] [review]
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
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?
(Assignee)

Comment 3

4 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

4 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 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

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2bd7bbea2d05
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2bd7bbea2d05
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.