Bug 263712 – Removing an item from rect (SVGTransformList) prevents attribute removal

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ahmad Saleem]

Hi Team,

While going through Blink's bug, I came across another where we differ:

Test Case: https://jsfiddle.net/b56f2v8e/

^ Console logs 'true' for STP181 while 'false' for Chrome Canary 120 and Firefox Nightly 121.

Chrome Bug: crbug.com/843901

Just wanted to raise so we can track and fix it.

Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/117840533>

[Karl Dubost]

Created attachment 468670 [details]
steps in devtools of safari and firefox

These are the steps in both browsers for the script. 

var elem = document.createElementNS('http://www.w3.org/2000/svg', 'rect');
elem.setAttribute('transform', 'matrix(1,0,0,1,0,0)');
elem.transform.baseVal.removeItem(0);
elem.removeAttribute('transform');
console.log(elem.hasAttribute('transform'));


The interesting differences are starting once the following has been applied. 
"elem.transform.baseVal.removeItem(0)"

Firefox: <rect transform=""></rect>
Safari:  <rect transform="matrix(1,0,0,1,0,0)"></rect>

then applying "elem.removeAttribute('transform')"
gives the same result

Firefox: <rect></rect>
Safari:  <rect></rect>

BUT doing "elem.hasAttribute('transform')"

Firefox: false
Safari:  true

which is not logical with the previous result.

[Karl Dubost]

it seems like there is not a complete synchronization of the element state.

[Nikolas Zimmermann]

Good catch Karl. Indeed this is the „lazy SVG attribute synchronization“ that apparently exposes this bug. We probably fail to unmap properly..

[Ahmad Saleem]

This fixes it:

void SVGElement::synchronizeAttribute(const QualifiedName& name)
{
    // If the value of the property has changed, serialize the new value to the attribute.
    if (auto value = propertyRegistry().synchronize(name)) {
        if (!value->isEmpty())
            setSynchronizedLazyAttribute(name, AtomString { *value });
    }
}

We can comment along the lines of - // Only set the attribute if it's not empty, otherwise the attribute should not exist.

@Nikolas & @Rob - if you think, it is right fix, let me know.

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/57109

[EWS]

merge-queue failed to commit PR to repository. To retry, remove any blocking labels and re-apply merge-queue label

[EWS]

Committed 306162@main (2867fa859ff6): <https://commits.webkit.org/306162@main>

Reviewed commits have been landed. Closing PR #57109 and removing active labels.