521682 - Inconsistent rendering when removing width from <svg:use>

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Based on layout/reftests/svg/dynamic-clipPath-01.svg

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Takeshi Kurosawa :takenspc]

Attachment: Patch

Use |mLengthAttributes[WIDTH|HEIGHT].GetAnimValueString()| instead of |GetAttr()|

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 406674 [details] [diff] [review]
Patch

>diff --git a/content/svg/content/src/nsSVGUseElement.cpp b/content/svg/content/src/nsSVGUseElement.cpp
> void
>-nsSVGUseElement::SyncWidthHeight(PRUint8 aAttrEnum)
>+nsSVGUseElement::SyncWidthHeight(PRUint8 aAttrEnum, PRBool aDoSetAttr)

There are two ways that length attributes can be set (or unset)
a) using setAttribute("width", ...
b) using element.width.baseVal.value = ...

aDoSetAttr distinguishes between the two cases. It is PR_TRUE in the first and PR_FALSE in the second. 

Our code internally looks like...

processChangeAttribute(PRBool aDoSetAttr) {
    ... do stuff to set the internal length
    if (aDoSetAttr) {
        call setAttribute to synchronise the internal SVG value with the DOM value
    }
}

aDoSetAttr is basically there to prevent an infinite loop, not about whether an attribute is set or unset.

> {
>   if (mClone && (aAttrEnum == WIDTH || aAttrEnum == HEIGHT)) {
>     nsCOMPtr<nsIDOMSVGSymbolElement> symbol = do_QueryInterface(mClone);
>     nsCOMPtr<nsIDOMSVGSVGElement>    svg    = do_QueryInterface(mClone);
> 
>     if (symbol || svg) {
>       if (aAttrEnum == WIDTH) {
>-        nsAutoString width;
>-        GetAttr(kNameSpaceID_None, nsGkAtoms::width, width);
>-        mClone->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);
>+        if (aDoSetAttr) {
>+          nsAutoString width;
>+          mLengthAttributes[WIDTH].GetAnimValueString(width);
>+          mClone->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);
>+        } else {
>+          mClone->UnsetAttr(kNameSpaceID_None, nsGkAtoms::width, PR_FALSE);
>+        }

... so this does not seem right. aDoSetAttr is irrelevant here and I think you just want this bit.

>+          nsAutoString width;
>+          mLengthAttributes[WIDTH].GetAnimValueString(width);
>+          mClone->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);

Comment 4

[Robert Longson [:longsonr]]

See comment 3 Takeshi.

Comment 5

[Takeshi Kurosawa :takenspc]

My patch is wrong. 
However we need to know whether <use> has width/height attribute when syncing clone's width/height.
For example,

<svg id="d" width="50%"/>
<use id="u" xlink:href="d" width="11%"/>

document.getElementById("u").removeAttribute(width); // width.baseVal.value is 0

clone's width should be 50%, not 0 (= <use>'s default value).
But in below, clone's width should be 0.

<svg id="d" width="50%"/>
<use id="u" xlink:href="d" width="11%"/>

document.getElementById("u").width.baseVal.value = 0;

Comment 6

[Robert Longson [:longsonr]]

Although HasAttribute can tell you whether there is an attribute, it may be that the clone's width is being animated without there being an attribute set i.e. it could have a non-zero width without an attribute.

I agree that the use cases should have the effect you state but I'm not sure you need to know there's an attribute or not to do that. No attribute has the same rendering as width="0" isn't it. Maybe I'm just not getting what the problem is?

Comment 7

[Takeshi Kurosawa :takenspc]

(In reply to comment #6)
> Although HasAttribute can tell you whether there is an attribute, it may be
> that the clone's width is being animated without there being an attribute set

|DidChangeLegth| is called before an attribute is actually removed/changed so |HasAttribute| returns old value.


> I agree that the use cases should have the effect you state but I'm not sure
> you need to know there's an attribute or not to do that. No attribute has the
> same rendering as width="0" isn't it. Maybe I'm just not getting what the
> problem is?


When referencing <symbol>, no attribute means "100%".
http://www.w3.org/TR/SVG11/struct.html#UseElement

> If the 'use'  element references a 'symbol'  element:
> (...)
>   If attributes width and/or height are not specified, the generated 'svg' element will use values of 100% for these attributes.

In |CreateAnonymousContent|, we copy only if there's an attribute.

>  if (symbol || svg) {
>    if (HasAttr(kNameSpaceID_None, nsGkAtoms::width)) {
>      nsAutoString width;
>      GetAttr(kNameSpaceID_None, nsGkAtoms::width, width);
>      newcontent->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);
>    }

# |GetAttr|... |CreateAnonymousContent| doesn't consider animations.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> (In reply to comment #6)
> > Although HasAttribute can tell you whether there is an attribute, it may be
> > that the clone's width is being animated without there being an attribute set
> 
> |DidChangeLegth| is called before an attribute is actually removed/changed so
> |HasAttribute| returns old value.
> 

You could hook into AfterSetAttribute instead of DidChangeLength then.

Comment 9

[Jesse Ruderman]

WFM?

Comment 10

[Robert Longson [:longsonr]]

I suppose that's possible. Other use tests you created have mysteriously started working recently. I suggest you check in the tests before it mysteriously breaks again.

Comment 11

[Jesse Ruderman]

in-testsuite+
http://hg.mozilla.org/mozilla-central/rev/08cb6bce6d83
http://hg.mozilla.org/mozilla-central/rev/7dd3b0ddd106