111492 - rx and ry should do proper range checking

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Håkan Waara]

The rx and ry attributes of <rect/> should do proper range checking, per the W3C
spec:

``If a properly specified value is provided for rx  but not for ry, then the
user agent processes the 'rect'  element with the effective value for ry  as
equal to rx. If a properly specified value is provided for ry  but not for rx,
then the user agent processes the 'rect'  element with the effective value for
rx  as equal to ry. 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, resulting in square corners. If rx  is greater than half of the width
of the rectangle, then the user agent processes the 'rect'  element with the
effective value for rx  as half of the width of the rectangle. If ry  is greater
than half of the height of the rectangle, then the user agent processes the
'rect'  element with the effective value for ry  as half of the height of the
rectangle.''

Patch coming up.

Comment 1

[Håkan Waara]

Attachment: Patch v1

Simple fix.  Alex, Bradley: mind taking a look or should I just CVS commit it
to the branch directly?

Comment 2

[Håkan Waara]

Bbaetz points out two issues:

1. We should also check for negativity of rx/ry, which is forbidden, according
to the spec. 

2. "How do we distinguish between an unset attribute and if you explicitely sets
it to 0?".

I don't know what error we should return for #1, but I have a patch coming up
that just cancels the function and thus makes the rect not render. Is that OK?

I don't know how to deal with #2.

Comment 3

[Håkan Waara]

Attachment: Patch v2

Error-out if rx or ry are negative.

Comment 4

[Håkan Waara]

Sorry, let me clarify problem #2:  If rx="0" is specified, we do not want to
make rx=ry, because we should only do that for unset attributes. How do we
figure out the difference between an unset and specified "0" attribute?

Comment 5

[Alex Fritze]

Some comments...
about #1:
The frame should never see negative values; the error processing needs to be 
somewhere higher up the chain (i.e. when someone assigns a negative value to 
the attribute/property). So I think we should have an assertion here in 
addition to the 'return'.

about #2: 
Hmm. I'm not sure I fully understand the specs here: If you specify rx="1" but 
nothing for ry and you read out the *javascript property* ry, should you get 1 
or 0? (I think you should get '1').
Anyway - the synchronisation between rx&ry  should probably be handled in the 
nsSVGRectElement (maybe in WillModifyObservable/DidModifyObservable). From 
there we can find out whether an attribute is set or not by asking the 
nsSVGAttributes, which keep two separate lists of (active) attributes and 
merely mapped attributes. 

Comment 6

[Bradley Baetz (:bbaetz)]

The following are not equivalent:

(1) <rect fill="red" x="10" y="10" width="100" height="100">
(2) <rect fill="red" x="10" y="10" rx="30" width="100" height="100">
(3) <rect fill="red" x="10" y="10" rx="30" ry="0" width="100" height="100">

(1) draws a rectangle with no rounded corners
(2) draws a rectangle with rx==ry, ie 30 px rounded corners
(3) draws a rectangle which looks identical to (1), but is actually six line
segments, with the "rounded" corners just plain lines (ry==0, so we follow the
rules in appendix F). It would look different with markers, but markers aren't
valid on <rect>, only <polygon> (and <path>, and ...). So it may be possivle to
optimise this into case 1 somewhere else, and not deal with it here. Whatever we
do, element.getAttribute() still has to return the correct val (null vs an
svglength of 0)

This interpretation matches what batik does, BTW.

How do you differentiate between case (2) and case (3)? Call into the nsIContent
directly, maybe?

Comment 7

[Alex Fritze]

Our implementation of element.getAttribute("ry") does the right thing for all 
three cases already. It knows how to differentiate. The way the logic works 
atm, is that an svg content element (i.e. the thing implementing nsIContent and 
all the DOM interfaces, e.g. nsSVGRectElement) registers a list of {attribute-
name, nsISVGValues}-pairs with its nsSVGAttributes object. E.g. in the case of 
the nsSVGRectangleElement, it registers {"ry", mRY}. mRY here is the 'live' ry 
value used by the content element, frame, etc. When you call 
nsSVGRectangleElement::GetRY(..) (or in JS: element.ry) the content element 
returns mRY. 
The way the attributes work is that when someone sets an attribute with 
name 'ry', the svgattributes-object a) goes and looks at its list of mapped 
{name, value-object}-pairs, b) finds {"rY", mRY}, c) moves this pair into a 
different list of 'actual' attributes and d) sets the value of mRY directly 
through a call of nsISVGValue::SetString(). 

Back to the problem here. Since element.ry is supposed to be 'live', what value 
should it (or rather element.ry.baseVal.value) be for the three cases?
This is important because depending on the answer, we want to handle the 
synchronisation between rx and ry either in the content-element or in the frame.

Comment 8

[Håkan Waara]

As confirmed by bbaetz and daniele, no one else does the "right thing" in these 
issues.

bbaetz to me on IRC:

<bbaetz> hwaara: anyway, if you've tested your patches, I'm happy for them to 
go in, given that noone else does this part of the dom, as long as you open up 
another bug on that issue.

I'll test my patch v2 later. afri, are you OK with this solution?

Comment 9

[Alex Fritze]

Absolutely.

Comment 10

[Håkan Waara]

Attachment: Patch v3

This patch does the local negativity checking, it assigns rx/ry into the other
value as needed, and checks so that the values are reasonable if set.

It's not perfect, it does not return a DOM exception and so on, but I'll spin
off a new bug on that.

Comment 11

[Håkan Waara]

Checked in to SVG_20010721_BRANCH.

Comment 12

[Håkan Waara]

See bugs #111991 and #111993 for the remaining issues.