Bug 136903 – changing SVG <defs> does not update SVG element.

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

[Damien Nozay [:dnozay]]

when changing the contents of an SVG <defs>, it does not update the SVG element.

example:
http://jsfiddle.net/dnozay/L1e76pts/

I have an empty <defs> tag which I later fill out once I compute some paths for the pattern.
In chrome this renders the pattern, in Safari this leaves the element black.

[Damien Nozay [:dnozay]]

Created attachment 238277 [details]
on safari.

[Damien Nozay [:dnozay]]

Created attachment 238278 [details]
on chrome.

[Damien Nozay [:dnozay]]

based on what I read there, this could be a namespace issue
http://stackoverflow.com/questions/24079659/loading-svg-using-innerhtml

so when I use innerHTML, it is probably not adding child elements correctly.
firebug lite confirms that there are no child elements.

[Sylvain Galineau]

Created attachment 241604 [details]
[WIP] fix.patch

The issue is that innerHTML (and outerHTML) are HTMLElement methods in WebKit. Per DOM Parsing [1], Chrome moved them to Element.

Source/WebCore/bindings/objc/PublicDOMInterfaces.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/Element.idl
Source/WebCore/html/HTMLElement.cpp
Source/WebCore/html/HTMLElement.h
Source/WebCore/html/HTMLElement.idl

The changes to the files above move innerHTML and outerHTML up the tree (which also fixes bug 135698). They fix the issue in a stand-alone SVG doc (see defs-innerHTML.svg). Nothing special about these changes except:
- I left innerText and outerText on HTMLElement; I believe these only exist for IE compat
- Both innerText/outerText and innerHTML/outerHTML use the mergeWithNextTextNode() helper. I made this a protected static on Element. 

Source/WebCore/html/parser/HTMLTreeBuilder.cpp
Source/WebCore/html/parser/HTMLTreeBuilder.h

These changes are needed to fix the original issue (see defs-innerHTML.html for a stand-alone non-JSfiddle version); they bring over the adjusted current node logic from Chrome, as defined by HTML [3]. Note that I tried to bring over all the HTMLTreeBuilder changes related to current node adjustment i.e. not just what I needed to fix this specific issue. This part definitely needs careful review.

[1] https://dvcs.w3.org/hg/innerhtml/raw-file/tip/index.html#innerhtml
[2] https://bugs.webkit.org/show_bug.cgi?id=135698
[3] // http://www.whatwg.org/specs/web-apps/current-work/#adjusted-current-node

[Sylvain Galineau]

Created attachment 241605 [details]
defs-innerHTML.html

A stand-alone version of the jsfiddle.

[Sylvain Galineau]

Created attachment 241606 [details]
defs-innerHTML.svg

An SVG version of defs-innerHTML.

[Damien Nozay [:dnozay]]

added a comment on bug 135698 to get interested parties' attention.

[Dean Jackson]

Comment on attachment 241604 [details]
[WIP] fix.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241604&action=review

Looks good. I suggest getting in the habit of writing the ChangeLog even when posting WIP patches because it helps people understand what is going on. "prepare-ChangeLog" or "webkit-patch prepare" are your friends.

Now you need to turn defs-innerHTML.svg into a test!

> Source/WebCore/html/HTMLElement.cpp:505
> -
> +    
>  void HTMLElement::setOuterText(const String& text, ExceptionCode& ec)

Oops. You inserted some space.

[Sylvain Galineau]

Created attachment 242347 [details]
[WIP] fix.patch

Updating initial patch per Dean's feedback

[Sylvain Galineau]

Created attachment 242351 [details]
[WIP] fix.patch

Fixing bad ChangeLog merge.

[Sylvain Galineau]

Created attachment 242353 [details]
[WIP] fix.patch

ChangeLog is not my friend; somehow crushed Oliver Hunt's update. Put it back in at the right spot in the timeline.

[WebKit Commit Bot]

Attachment 242353 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Element.h:580:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2847:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:19:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 242353 [details]
[WIP] fix.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242353&action=review

Very close. Just some merge issues, and minor questions about the test modifications.

> LayoutTests/ChangeLog:13
> [phone]  Sylvain Galineau  <[e-mail]>
> +
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * js/dom/dom-static-property-for-in-iteration-expected.txt:
> +        * platform/mac/svg/in-html/svg-inner-html-expected.png: Added.
> +        * svg/css/svg-attribute-length-parsing-expected.txt:
> +        * svg/in-html/svg-inner-html-expected.txt: Added.
> +        * svg/in-html/svg-inner-html.html: Added.
> +

You need to fill in the OOPS! lines here - at least the description and the bug.

For the s/dom/dom-static-property-for-in-iteration-expected.txt line, you should probably explain why this changed. e.g. You moved innerHTML and outerHTML onto Element.

And I think this line is wrong:

platform/mac/svg/in-html/svg-inner-html-expected.png: Added

Didn't you remove that image now that it is a text test?

> LayoutTests/svg/css/svg-attribute-length-parsing-expected.txt:2
> +Test
>  Test CSS parsing on SVG presentation attributes.

Was this expected to change? I don't see the actual test changing.

> Source/WebCore/ChangeLog:13
> -2014-12-01  Bartlomiej Gajda  <[e-mail]>
> [phone]  Sylvain Galineau  <[e-mail]>
>  
> -        [MSE] Fix not always calling mediaPlayer seek.
> -        https://bugs.webkit.org/show_bug.cgi?id=139139
> +        No support for innerHTML on SVGElement
> +        https://bugs.webkit.org/show_bug.cgi?id=136903
>  
> -        Reviewed by Jer Noble.
> +        Reviewed by NOBODY (OOPS!).
>  
> -        Original comment states that media source shall always be notified of seek if it's not closed.
> +        Two parts to this patch:
> +        1. Move innerHTML/outerHTML to Element so SVG elements can inherit them, per https://dvcs.w3.org/hg/innerhtml/raw-file/tip/index.html#innerhtml
> +        2. Make sure fragment insertion is processed relative to the proper node, per http://www.whatwg.org/specs/web-apps/current-work/#adjusted-current-node
>  
> -        No new tests needed.
> +        The latter part was ported over from Blink. 
>  

Something really crazy is going on here. I suggest you revert all your changes to ChangeLog, update, then call prepare-ChangeLog again and add your content back.

> Source/WebCore/ChangeLog:16
> +        * bindings/objc/PublicDOMInterfaces.h: Move innerHTML/outerHTML to Element

Nit: Period at end of sentence.

[Sylvain Galineau]

Created attachment 242361 [details]
[WIP] fix.patch


> You need to fill in the OOPS! lines here - at least the description and the bug.

Yes, my bad. I missed the LayoutTests ChangeLog :( Fixed.

>For the s/dom/dom-static-property-for-in-iteration-expected.txt line, 
>you should probably explain why this changed. e.g. You moved innerHTML 
>and outerHTML onto Element.

Done.

>And I think this line is wrong:

>platform/mac/svg/in-html/svg-inner-html-expected.png: Added

>Didn't you remove that image now that it is a text test?

This was intentional. The reason: when I started working on this I was quickly able to get innerHTML to return the proper string even though it failed to render properly in an HTML document. So I think we want to test that both 1) innerHTML does the right thing DOM-wise - here I'm checking the innerHTML of the <pattern> added through defs.innerHTML - and 2) that the proper renderer gets invoked to make the rectangle green. Is there a better way? Do I also need a .svg version of the test?

>> LayoutTests/svg/css/svg-attribute-length-parsing-expected.txt:2
>> +Test
>>  Test CSS parsing on SVG presentation attributes.

>Was this expected to change? I don't see the actual test changing.

The test is unchanged, but the expected result is different. The test code in svg-attribute-length-parsin.js writes 'Test' to the innerHTML of an SVG element it creates. This was a no-op in the past but obviously no longer is; thus I believe this new expected.txt to be the correct one. Alternatively, I could fix the JS code to not add that innerHTML 'Test' string but I wasn't sure if this might have other side-effects.

> -        No new tests needed.
> +        The latter part was ported over from Blink. 
>  

>Something really crazy is going on here. I suggest you revert all your changes to >ChangeLog, update, then call prepare-ChangeLog again and add your content back.

Something really crazy seems the proper technical term. Couldn't make sense of it either so reverted and added content back in.


>Nit: Period at end of sentence.

Done.

[WebKit Commit Bot]

Attachment 242361 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Element.h:580:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2847:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 242361 [details]
[WIP] fix.patch

Clearing flags on attachment: 242361

Committed r176630: <http://trac.webkit.org/changeset/176630>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sylvain Galineau]

*** Bug 135698 has been marked as a duplicate of this bug. ***

Can anyone confirm what version this fix is/will be included in?