886390 - Assigning to innerHTML on an SVG element should create elements in the SVG namespace

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: testcase

See attached testcase. I am assigning a SVG filter's innerHTML to itself. Expected result: should make no difference, right? Actual result: causes the filtered element to disappear from the page.

I'm testing on linux 64bit. Tried with and without GL layers.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: testcase (made more explicit)

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: testcase, outputting the inner element's xml namespace

Thanks to jwatt for pointing this out. I get:

[13:53:50.403] "Before: <feGaussianBlur id="blur-filter1" stdDeviation="5"></feGaussianBlur>"
[13:53:50.403] "ns: http://www.w3.org/2000/svg"
[13:53:50.404] "After: <fegaussianblur id="blur-filter1" stddeviation="5"></fegaussianblur>"
[13:53:50.404] "ns: http://www.w3.org/1999/xhtml"

Thus, assigning innerHTML to itself causes the namespaceURI of the inner element to change. Is that expected behavior? Does it explain what I'm seeing?

Comment 3

[Jonathan Watt [:jwatt]]

Yes, giving the contents of the filter a non-SVG namespace would be expected to result in the filtered elements rendering nothing at all.

Anne, what's the current status on fixing innerHTML to create elements in the same namespace as the element that .innerHTML is being assigned to?

Comment 4

[Anne (:annevk)]

This was fixed in the HTML standard through https://www.w3.org/Bugs/Public/show_bug.cgi?id=17924

Comment 5

[Jonathan Watt [:jwatt]]

Anne: prefect, thanks.

Benoit: so it looks like you can't use innerHTML in the way that you want until the HTML5 spec changes are implemented.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the information! Feel free to close this bug if it's not useful now.

Comment 7

[Jonathan Watt [:jwatt]]

Let's leave this open as the bug to implement the HTML5 spec changes for .innerHTML.

Comment 8

[Thaddee Tyl [:espadrine]]

Attachment: minimal testcase, shows how this bug affects DOM order

Comment 9

[Jonathan Watt [:jwatt]]

Any plans to fix the HTML5 parser?

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> Any plans to fix the HTML5 parser?

I'll be away for the next two weeks. Additionally, I have a higher-priority project to deal with. I intend to go back to fixing parser bugs when the higher-priority project permits. I'm not sure if that means August or some time next year. :-(

This shouldn't be a big change, though, so in that sense, it might be possible to interleave this with other stuff in August.

Comment 13

[Robert Longson [:longsonr]]

Attachment: patch

Comment 14

[Robert Longson [:longsonr]]

Attachment: https://hg.mozilla.org/projects/htmlparser patch

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 8474226 [details] [diff] [review]
https://hg.mozilla.org/projects/htmlparser patch

The "adjusted current node" concept introduced in html5.org/tools/web-apps-tracker?from=7767&to=7768 seems to care about MathML text integration points, annotation-xml and HTML integration points but the patch seems to use <math> or <svg> as a placeholder for all these. Is the patch in error or is there some reason that I fail to see why it's OK to use just <math> and <svg> without special-casing mi, mo, mn, ms, mtext, annotation-xml from MathML and foreignObject, desc and title from SVG?

Is it intentional that the patch doesn't attempt to implement http://html5.org/tools/web-apps-tracker?from=7917&to=7918 ? It probably should.

Comment 16

[Robert Longson [:longsonr]]

Some expanded hints as to what to do would be appreciated as this is my first patch in this area. I'm sure your review comments make sense to you but they go right over my head at the moment.

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Robert Longson from comment #16)
> Some expanded hints as to what to do would be appreciated as this is my
> first patch in this area. I'm sure your review comments make sense to you
> but they go right over my head at the moment.

If the context is e.g. foreignObject in the SVG namespace, the code should probably assign
elementName = ElementName.FOREIGNOBJECT. Similarly for all the special MathML and SVG elements that can take HTML children.

Or that's what I *think* is needed unless for some special reason I fail to see it's not. The spec text could be clearer.

Comment 19

[Robert Longson [:longsonr]]

Attachment: https://hg.mozilla.org/projects/htmlparser patch

Comment 20

[Robert Longson [:longsonr]]

Attachment: mozilla patch

Comment 21

[Henri Sivonen (:hsivonen)]

Attachment: Turn off the quirky handling of HTML name overlaps when in the fragment mode

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> Is it intentional that the patch doesn't attempt to implement
> http://html5.org/tools/web-apps-tracker?from=7917&to=7918 ? It probably
> should.

Here's a patch that's supposed to implement that part.

(I don't feel comfortable reviewing the other patch by just looking at the code, so I'm trying out stuff.)

Comment 22

[Henri Sivonen (:hsivonen)]

Attachment: Turn off the quirky handling of HTML name overlaps when in the fragment mode,v2

Sigh. This gets ugly fast. The line wrapping is truly weird, but I don't want to fight the Eclipse formatter.

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 8481788 [details] [diff] [review]
https://hg.mozilla.org/projects/htmlparser patch

FWIW, the Blink change is much closer to the spec text:
https://chromium.googlesource.com/chromium/blink/+/a8a0a83498e1d5f7014c79cffa7e45fda6a8b08a%5E!/#F18

Comment 24

[Henri Sivonen (:hsivonen)]

Sorry about the slow review here. Convincing myself that the patch is equivalent with the spec text turns out to be harder than I expected.

Comment 25

[Henri Sivonen (:hsivonen)]

An update of what's happening: I'm working on making the testing infrastructure be able to test this, so that I can test the patch.

Comment 26

[Henri Sivonen (:hsivonen)]

Another update: It seems that the attached patch doesn't work right with foreignObject context. I'm working on a new patch.

Comment 27

[Henri Sivonen (:hsivonen)]

Comment on attachment 8481788 [details] [diff] [review]
https://hg.mozilla.org/projects/htmlparser patch

I tested this, and stuff didn't work, because the wrong StackNode<T> constructor is used. Also, there are further complications. I'm far enough trying to figure those out, that I should probably just take this bug.

Comment 28

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #27)
> I'm far enough
> trying to figure those out, that I should probably just take this bug.

Taking for that reason. (Sorry, I know this is not really how reviews are supposed to go.)

Comment 29

[Henri Sivonen (:hsivonen)]

Attachment: Adjusted current node, Java patch

Comment 30

[Henri Sivonen (:hsivonen)]

Attachment: Adjusted current node, Gecko patch

Comment 31

[Henri Sivonen (:hsivonen)]

Attachment: Turn off the quirky handling of HTML name overlaps when in the fragment mode,v2, Gecko version

Comment 32

[Henri Sivonen (:hsivonen)]

Attachment: Import html5lib tests for this

The html5lib files come from https://github.com/html5lib/html5lib-tests/pull/50

Comment 33

[Henri Sivonen (:hsivonen)]

Sigh. The patches here deviate from the spec and Blink when the context node's namespace isn't any of HTML, SVG or MathML.

Comment 34

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=7c2309ebec07

Comment 35

[Henri Sivonen (:hsivonen)]

Attachment: Part 4: Adjust an old test

https://tbpl.mozilla.org/?tree=Try&rev=d46ca766bfdb

Comment 36

[Henri Sivonen (:hsivonen)]

Comment on attachment 8482252 [details] [diff] [review]
Turn off the quirky handling of HTML name overlaps when in the fragment mode,v2

Blink hasn't implemnted this spec change, yet.

Comment 37

[Henri Sivonen (:hsivonen)]

Comment on attachment 8488509 [details] [diff] [review]
Adjusted current node, Java patch

There's no need to add an "adjusted current node". Instead, AFAICT, all cases where the spec looks for the fictional "html" root in the fragment case, we check the stack index (0) instead. Therefore, there's not need to adjust the root of the stack during the parse. We can just stick a stack node corresponding to the adjusted current node on the stack.

This patch doesn't match the spec when the context node is in a namespace that's none of HTML, SVG or MathML. Hixie said the specced behavior is accidental, so let's not block this bug on the resolution of https://www.w3.org/Bugs/Public/show_bug.cgi?id=26804 .

The context namespace is not nulled out when the context local name is, because our namespaces are int32_t and, therefore, don't leak and I don't want trouble from the translation trying to assigning nullptr to int32_t.

Comment 38

[William Chen [:wchen]]

Comment on attachment 8482252 [details] [diff] [review]
Turn off the quirky handling of HTML name overlaps when in the fragment mode,v2

Review of attachment 8482252 [details] [diff] [review]:
-----------------------------------------------------------------

::: src/nu/validator/htmlparser/impl/TreeBuilder.java
@@ +1659,5 @@
>                                  continue starttagloop;
> +                            } // else fall thru
> +                        case FONT:
> +                            // recheck group == FONT to deal with fall-through.
> +                            // sigh.

We could also combine the FONT case with the other cases and change the code to something like:

errHtmlStartTagInForeignContext(name);
if (!fragment && (group != FONT ||
        attributes.contains(AttributeName.COLOR) ||
        attributes.contains(AttributeName.FACE) ||
        attributes.contains(AttributeName.SIZE))) {
    while (!isSpecialParentInForeign(stack[currentPtr])) {
        pop();
    }
    continue starttagloop;
} // else fall thru

This gets rid of some code duplication and is less weird with the fall through. It's essentially the same except it does the group check in the !fragment case instead of the fragment/fall through case. But I'll leave it up to you to decide which way is better.

Comment 39

[William Chen [:wchen]]

Comment on attachment 8488509 [details] [diff] [review]
Adjusted current node, Java patch

Review of attachment 8488509 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see the last change in https://html5.org/tools/web-apps-tracker?from=7767&to=7768 in this patch (changes to the "Any other end tag" in foreign content, https://html.spec.whatwg.org/multipage/syntax.html#parsing-main-inforeign). I think the intent of that change was to prevent </html> from popping off the last node in foreign content, but in our implementation the top of the stack isn't necessarily <html>, instead the top of the stack could be svg, math, foreignObject, etc. and we don't want a corresponding end tag to pop that node.

::: src/nu/validator/htmlparser/impl/TreeBuilder.java
@@ +617,5 @@
> +                        || "foreignObject" == contextName) {
> +                    // These elements are all alike and we don't care about
> +                    // the exact name.
> +                    elementName = ElementName.FOREIGNOBJECT;
> +                }

This is putting an approximation of the adjusted current node at stack[0] when the spec puts <html> at stack[0], I think this discrepancy should at least be commented somewhere in this function. I also feel like this will affect some other algorithms in the parser.

@@ +1514,5 @@
>                  case IN_CAPTION:
>                  case IN_CELL:
>                  case IN_BODY:
>                      // [NOCPP[
> +                    openelementloop: for (int i = currentPtr; i > 0; i--) {

Is this something we need to change now that stack[0] isn't always <html> in the fragment case for foreign contexts? If so, I think there should be a comment here explaining that.

Comment 40

[James Graham [:jgraham]]

Comment on attachment 8488528 [details] [diff] [review]
Import html5lib tests for this

Review of attachment 8488528 [details] [diff] [review]:
-----------------------------------------------------------------

Needs the updated version of foreign-fragment.dat but otherwise seems OK.

Comment 43

[Henri Sivonen (:hsivonen)]

Attachment: Part 3: Import html5lib tests

(In reply to James Graham [:jgraham] from comment #40)
> Needs the updated version of foreign-fragment.dat but otherwise seems OK.

Thanks. This patch imports the updated foreign-fragment.dat.

Comment 44

[Henri Sivonen (:hsivonen)]

Attachment: Part 2: Turn off the quirky handling of HTML name overlaps when in the fragment mode, v3, Gecko version

Comment 45

[Henri Sivonen (:hsivonen)]

Attachment: Turn off the quirky handling of HTML name overlaps when in the fragment mode, v3, Java version

Comment 46

[Henri Sivonen (:hsivonen)]

Attachment: Part 1: Adjusted current node, Gecko version, v2

Comment 47

[Henri Sivonen (:hsivonen)]

Attachment: Adjusted current node, Java patch, v2

Comment 48

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a762506dae7

Comment 49

[Henri Sivonen (:hsivonen)]

Thanks. Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca1182ed15d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd43e673c9b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ce8ff7c4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9137aade756

https://hg.mozilla.org/projects/htmlparser/rev/3723920b828b
https://hg.mozilla.org/projects/htmlparser/rev/0cab889462bf

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fca1182ed15d
https://hg.mozilla.org/mozilla-central/rev/fd43e673c9b4
https://hg.mozilla.org/mozilla-central/rev/f65ce8ff7c4d
https://hg.mozilla.org/mozilla-central/rev/a9137aade756