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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: Håkan Waara, Assigned: jwatt)

Tracking

Trunk
x86
All
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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".
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 www-svg@w3.org, 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?
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

16 years ago
Created attachment 59231 [details]
A test case that works with Mozilla

Comment 4

16 years ago
Created attachment 59232 [details]
A test case that work with Adobe SVG Viewer
(Assignee)

Comment 5

14 years ago
bbaetz, did you ever send that email to the www-svg@w3.org?
Can't remmeber, sorry - too long ago.
(Assignee)

Comment 7

14 years ago
Mass reassign of SVG bugs that aren't currently being worked on by Alex to
general@svg.bugs. 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. 
Assignee: dean.jackson → general
(Assignee)

Updated

13 years ago
Assignee: general → jonathan.watt
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
QA Contact: dean.jackson → ian
(Assignee)

Comment 9

13 years ago
Created attachment 167073 [details] [diff] [review]
patch without forgotten comment
Attachment #167071 - Attachment is obsolete: true
(Assignee)

Comment 10

13 years ago
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.
Attachment #167073 - Flags: review?(tor)
(Assignee)

Comment 11

13 years ago
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.
Attachment #167073 - Attachment is obsolete: true
Attachment #167073 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
Depends on: 270257
(Assignee)

Comment 13

13 years ago
Created attachment 168824 [details] [diff] [review]
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?
Attachment #168824 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
Blocks: 274886
(Assignee)

Updated

13 years ago
No longer depends on: 270257

Comment 14

13 years ago
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.
Attachment #168824 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
Attachment #168824 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
No longer blocks: 274886
Depends on: 274886
(Assignee)

Comment 15

13 years ago
Created attachment 171510 [details]
testcase

The HasAttr problem is fixed. Here's a testcase to run my patch over.
Attachment #59231 - Attachment is obsolete: true
Attachment #59232 - Attachment is obsolete: true

Comment 16

13 years ago
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
Attachment #168824 - Flags: review?(tor) → review+

Comment 18

13 years ago
Comment on attachment 171810 [details] [diff] [review]
new patch

Looks good to me.
Attachment #171810 - Flags: review+

Comment 19

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.