111993 - Rx and ry attributes of SVG <rect/> element should distinguish between unset attribute and "0"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Håkan Waara]

Spun off bug #111492...

Currently, if the user explicitely sets the value rx="0" for example, we think 
that rx is unset, so if ry is set we do |rx = ry| and thus ignore the user's rx 
value.

Testcase:
<rect x="1" y="1" style="fill: red" width="100" height="200" rx="0" rx="30"/>

Actual result: rx will be "30" because we think it's unset (we do not 
distinguish between 0 and unset attributes).

Expected result: that rx will stay as "0".

Comment 1

[Bradley Baetz (:bbaetz)]

hwaara's test case should be: <rect x="1" y="1" style="fill: red" width="100"
height="200" rx="0" ry="30"/> - that was just a typo

Fixing this depends on me mailing [e-mail], pointing to daniele's test
case. I'll attach a pointer to the archives once that it done.

Daniele - can you please attach two separate versions of your test here, one
with the standard <script> tag, and the other with the <html:script> variant we
currently require.

Summary of irc discussion for a test case w/o rx set at all, but with ry="30"
(see daniele's tests):

What should the dom stuff return for foo.rx? batik returns "unimplemented" for
foo.rx, while the adobe viewer doesn't support it either, instead returning
"undefined". I don't know of a viewer which does support this. Do we return:

a) null/undefined from foo.rx - ie the standard js undefined attr. Rationale: It
wasn't defined

b) throw some form of NOT_FOUND exception. Rationale: None, really, it was just
another option. This option is differnet to how the rest of the DOM works; it
was just added for completeness. I don't think that this solution is viable.

c) Return an rx with foo.rx.baseVal.unitType of SVG_LENGTHTYPE_UNKNOWN.
Rationale: Similarity to Section 1.6.1 of the DOM2-HTML spec, which states: "The
return value of an attribute that is unspecified and does not have a default
value is the empty string if the return type is a DOMString, false if the return
type is a boolean and 0 if the return type is a number.", assuming no default
value, which is implied from SVG 9.2: "If neither rx nor ry has a properly
specified value, then the user agent processes the 'rect' element as if no
rounding had been specified".

d) Return an rx with foo.rx.baseVal == foo.ry.baseVal. Rationale: Same as (c),
taking the default value to be ry

e) Return an rx with foo.rx.baseVal.value == 0. Rationale: Same as (c), taking a
default value of 0, which is the effect of the sentence quoted from section 9.2.

f) Something else. Rationale: None of the above solutions are really complete.

In all cases, especialy d, and e, consider what happens if I add 1 to
foo.rx.baseVal.value vs foo.setAttribute('rx',1+foo.getAttribute('rx')); Should
they be the same? The answer to this may impact animations, but we're ignoring
animation in all this, mainly because mozilla deosn't implement that yet.

Comments before I send this off?

Comment 2

[Bradley Baetz (:bbaetz)]

Actually, I though of another solution, which I like best:

rx _always_ returns an valid animatedLength, but .baseVal will return null if
its not specified. This allows for <set> to work, I think.

Comments?

Comment 3

[Daniele Nicolodi]

Attachment: A test case that works with Mozilla

Comment 4

[Daniele Nicolodi]

Attachment: A test case that work with Adobe SVG Viewer

Comment 5

[Jonathan Watt [:jwatt]]

bbaetz, did you ever send that email to the [e-mail]?

Comment 6

[Bradley Baetz (:bbaetz)]

Can't remmeber, sorry - too long ago.

Comment 7

[Jonathan Watt [:jwatt]]

Mass reassign of SVG bugs that aren't currently being worked on by Alex to
[e-mail]. If you think someone should be assigned to your bug you can
join the #svg channel on mozilla.org's IRC server ( irc://irc.mozilla.org/svg )
where you can try to convince one of the SVG hackers to work on it. We aren't
always there, so if you don't get a response straight away please try again later. 

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch without forgotten comment

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 167073 [details] [diff] [review]
patch without forgotten comment

This bug is not the place to address what properties should return null or
whatever when an attribute is or isn't present. This patch only ensures that rx
is set to ry when the attribute rx isn't present and vice versa.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 167073 [details] [diff] [review]
patch without forgotten comment

Actually this isn't working. I wasn't interpreting the testcase correctly last
night. Both hasRx and hasRy are being set to PRTrue.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: better patch

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: better patch + other improvements to the file

Tim, this patch fixes up some other issues I have with this file as well as
patching the bug at hand. I'd prefer this patch to go in rather than the
previous one. Can you review it please?

Comment 14

[tor]

Comment on attachment 168824 [details] [diff] [review]
better patch + other improvements to the file

Don't want to review until the HasAttr problem is fixed.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: testcase

The HasAttr problem is fixed. Here's a testcase to run my patch over.

Comment 16

[Scooter Morris]

Comment on attachment 168824 [details] [diff] [review]
better patch + other improvements to the file

Looks good except for an inconsistent use of NS_WARNING and NS_ASSERTION when
do_queryInterface fails.  Should probably only do one.

r+ with that fixed

Comment 17

[Jonathan Watt [:jwatt]]

Attachment: new patch

Comment 18

[Scooter Morris]

Comment on attachment 171810 [details] [diff] [review]
new patch

Looks good to me.

Comment 19

[tor]

Checked in.

Checking in nsSVGRectFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGRectFrame.cpp,v  <--  nsSVGRectFrame.cpp
new revision: 1.6; previous revision: 1.5
done

Comment 20

[Jonathan Watt [:jwatt]]

Checked in reftest:
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/bugs/111993.svg