Bug 136903 - changing SVG <defs> does not update SVG element.
Summary: changing SVG <defs> does not update SVG element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 135698 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-09-17 18:11 PDT by Damien Nozay [:dnozay]
Modified: 2015-06-24 11:54 PDT (History)
4 users (show)

See Also:


Attachments
on safari. (5.42 KB, image/png)
2014-09-17 18:11 PDT, Damien Nozay [:dnozay]
no flags Details
on chrome. (5.84 KB, image/png)
2014-09-17 18:12 PDT, Damien Nozay [:dnozay]
no flags Details
[WIP] fix.patch (15.50 KB, patch)
2014-11-14 11:42 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff
defs-innerHTML.html (655 bytes, text/html)
2014-11-14 11:43 PST, Sylvain Galineau
no flags Details
defs-innerHTML.svg (639 bytes, image/svg+xml)
2014-11-14 11:44 PST, Sylvain Galineau
no flags Details
[WIP] fix.patch (28.36 KB, patch)
2014-12-01 14:54 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff
[WIP] fix.patch (25.05 KB, patch)
2014-12-01 15:00 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff
[WIP] fix.patch (26.89 KB, patch)
2014-12-01 15:08 PST, Sylvain Galineau
dino: review-
Details | Formatted Diff | Diff
[WIP] fix.patch (22.99 KB, patch)
2014-12-01 16:47 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Damien Nozay [:dnozay] 2014-09-17 18:11:16 PDT
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.
Comment 1 Damien Nozay [:dnozay] 2014-09-17 18:11:41 PDT
Created attachment 238277 [details]
on safari.
Comment 2 Damien Nozay [:dnozay] 2014-09-17 18:12:15 PDT
Created attachment 238278 [details]
on chrome.
Comment 3 Damien Nozay [:dnozay] 2014-09-17 18:40:34 PDT
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.
Comment 4 Sylvain Galineau 2014-11-14 11:42:26 PST
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
Comment 5 Sylvain Galineau 2014-11-14 11:43:16 PST
Created attachment 241605 [details]
defs-innerHTML.html

A stand-alone version of the jsfiddle.
Comment 6 Sylvain Galineau 2014-11-14 11:44:00 PST
Created attachment 241606 [details]
defs-innerHTML.svg

An SVG version of defs-innerHTML.
Comment 7 Damien Nozay [:dnozay] 2014-11-14 12:05:27 PST
added a comment on bug 135698 to get interested parties' attention.
Comment 8 Dean Jackson 2014-11-14 15:54:52 PST
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.
Comment 9 Sylvain Galineau 2014-12-01 14:54:09 PST
Created attachment 242347 [details]
[WIP] fix.patch

Updating initial patch per Dean's feedback
Comment 10 Sylvain Galineau 2014-12-01 15:00:56 PST
Created attachment 242351 [details]
[WIP] fix.patch

Fixing bad ChangeLog merge.
Comment 11 Sylvain Galineau 2014-12-01 15:08:27 PST
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.
Comment 12 WebKit Commit Bot 2014-12-01 16:05:18 PST
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.
Comment 13 Dean Jackson 2014-12-01 16:12:49 PST
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
> +2014-12-01  Sylvain Galineau  <galineau@adobe.com>
> +
> +        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  <b.gajda@samsung.com>
> +2014-12-01  Sylvain Galineau  <galineau@adobe.com>
>  
> -        [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.
Comment 14 Sylvain Galineau 2014-12-01 16:47:46 PST
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.
Comment 15 WebKit Commit Bot 2014-12-02 02:01:35 PST
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.
Comment 16 WebKit Commit Bot 2014-12-02 02:49:50 PST
Comment on attachment 242361 [details]
[WIP] fix.patch

Clearing flags on attachment: 242361

Committed r176630: <http://trac.webkit.org/changeset/176630>
Comment 17 WebKit Commit Bot 2014-12-02 02:49:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Sylvain Galineau 2014-12-15 10:01:25 PST
*** Bug 135698 has been marked as a duplicate of this bug. ***
Comment 19 rmcerlean 2015-06-24 11:54:17 PDT
Can anyone confirm what version this fix is/will be included in?