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

RESOLVED FIXED in mozilla36

Status

()

Core
SVG
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bjacob, Assigned: hsivonen)

Tracking

({dev-doc-needed})

Trunk
mozilla36
x86_64
Linux
dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 12 obsolete attachments)

935 bytes, text/html
Details
393 bytes, text/html
Details
1.92 KB, patch
wchen
: review+
Details | Diff | Splinter Review
15.75 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
4.75 KB, patch
wchen
: review+
Details | Diff | Splinter Review
2.78 KB, patch
wchen
: review+
Details | Diff | Splinter Review
24.00 KB, patch
wchen
: review+
Details | Diff | Splinter Review
12.91 KB, patch
wchen
: review+
Details | Diff | Splinter Review
Created attachment 766763 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 766767 [details]
testcase (made more explicit)
(Reporter)

Updated

4 years ago
Attachment #766763 - Attachment is obsolete: true
(Reporter)

Comment 2

4 years ago
Created attachment 766770 [details]
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?
Attachment #766767 - Attachment is obsolete: true
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?
Summary: Assigning to innerHTML of a SVG filter breaks it → Assigning to innerHTML of a SVG filter causes the filtered elements to disappear

Comment 4

4 years ago
This was fixed in the HTML standard through https://www.w3.org/Bugs/Public/show_bug.cgi?id=17924
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.
(Reporter)

Comment 6

4 years ago
Thanks for the information! Feel free to close this bug if it's not useful now.
Let's leave this open as the bug to implement the HTML5 spec changes for .innerHTML.
Component: SVG → HTML: Parser
OS: Linux → All
Hardware: x86_64 → All
Summary: Assigning to innerHTML of a SVG filter causes the filtered elements to disappear → Assigning to innerHTML on an SVG element should create elements in the SVG namespace
Component: HTML: Parser → SVG
OS: All → Linux
Hardware: All → x86_64
Created attachment 8423835 [details]
minimal testcase, shows how this bug affects DOM order
Attachment #8423835 - Attachment mime type: text/plain → text/html
Any plans to fix the HTML5 parser?
Flags: needinfo?(hsivonen)
Blocks: 1034495
Duplicate of this bug: 1034475
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1034495
(Assignee)

Comment 12

3 years ago
(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.
Flags: needinfo?(hsivonen)
Created attachment 8474224 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #8474224 - Flags: review?(hsivonen)
Created attachment 8474226 [details] [diff] [review]
https://hg.mozilla.org/projects/htmlparser patch
Attachment #8474226 - Flags: review?(hsivonen)
(Assignee)

Comment 15

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8474224 - Flags: review?(hsivonen) → review-
(Assignee)

Updated

3 years ago
Attachment #8474226 - Flags: review?(hsivonen) → review-
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.
Flags: needinfo?(hsivonen)
Duplicate of this bug: 1059560
(Assignee)

Comment 18

3 years ago
(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.
Flags: needinfo?(hsivonen)
Created attachment 8481788 [details] [diff] [review]
https://hg.mozilla.org/projects/htmlparser patch
Attachment #8474226 - Attachment is obsolete: true
Attachment #8481788 - Flags: review?(hsivonen)
Created attachment 8481789 [details] [diff] [review]
mozilla patch
Attachment #8474224 - Attachment is obsolete: true
Attachment #8481789 - Flags: review?(hsivonen)
(Assignee)

Comment 21

3 years ago
Created attachment 8482247 [details] [diff] [review]
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.)
(Assignee)

Comment 22

3 years ago
Created attachment 8482252 [details] [diff] [review]
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.
Attachment #8482247 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
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
(Assignee)

Comment 24

3 years ago
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.
(Assignee)

Comment 25

3 years ago
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.
(Assignee)

Comment 26

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

Comment 27

3 years ago
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.
Attachment #8481788 - Flags: review?(hsivonen) → review-
(Assignee)

Updated

3 years ago
Attachment #8481789 - Flags: review?(hsivonen) → review-
(Assignee)

Comment 28

3 years ago
(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.)
Assignee: longsonr → hsivonen
(Assignee)

Comment 29

3 years ago
Created attachment 8488509 [details] [diff] [review]
Adjusted current node, Java patch
Attachment #8481788 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8488510 [details] [diff] [review]
Adjusted current node, Gecko patch
Attachment #8481789 - Attachment is obsolete: true
(Assignee)

Comment 31

3 years ago
Created attachment 8488516 [details] [diff] [review]
Turn off the quirky handling of HTML name overlaps when in the fragment mode,v2, Gecko version
(Assignee)

Comment 32

3 years ago
Created attachment 8488528 [details] [diff] [review]
Import html5lib tests for this

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

Comment 33

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

Comment 34

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7c2309ebec07
(Assignee)

Comment 35

3 years ago
Created attachment 8489330 [details] [diff] [review]
Part 4: Adjust an old test

https://tbpl.mozilla.org/?tree=Try&rev=d46ca766bfdb
Attachment #8489330 - Flags: review?(wchen)
(Assignee)

Comment 36

3 years ago
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.
Attachment #8482252 - Flags: review?(wchen)
(Assignee)

Comment 37

3 years ago
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.
Attachment #8488509 - Flags: review?(wchen)
(Assignee)

Updated

3 years ago
Attachment #8488510 - Flags: review?(wchen)
(Assignee)

Updated

3 years ago
Attachment #8488516 - Flags: review?(wchen)
(Assignee)

Updated

3 years ago
Attachment #8488528 - Flags: review?(james)
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.
Attachment #8482252 - Flags: review?(wchen) → review+

Updated

3 years ago
Attachment #8489330 - Flags: review?(wchen) → review+
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.
Attachment #8488509 - Flags: review?(wchen) → review-

Updated

3 years ago
Attachment #8488516 - Flags: review?(wchen) → review+

Updated

3 years ago
Attachment #8488510 - Flags: review?(wchen)
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.
Attachment #8488528 - Flags: review?(james) → review+
Duplicate of this bug: 1080471
Duplicate of this bug: 1081890
(Assignee)

Comment 43

3 years ago
Created attachment 8505373 [details] [diff] [review]
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.
Attachment #8488528 - Attachment is obsolete: true
Attachment #8505373 - Flags: review+
(Assignee)

Comment 44

3 years ago
Created attachment 8505377 [details] [diff] [review]
Part 2: Turn off the quirky handling of HTML name overlaps when in the fragment mode, v3, Gecko version
Attachment #8488516 - Attachment is obsolete: true
Attachment #8505377 - Flags: review?(wchen)
(Assignee)

Comment 45

3 years ago
Created attachment 8505382 [details] [diff] [review]
Turn off the quirky handling of HTML name overlaps when in the fragment mode, v3, Java version
Attachment #8482252 - Attachment is obsolete: true
Attachment #8505382 - Flags: review?(wchen)
(Assignee)

Comment 46

3 years ago
Created attachment 8505385 [details] [diff] [review]
Part 1: Adjusted current node, Gecko version, v2
Attachment #8488510 - Attachment is obsolete: true
Attachment #8505385 - Flags: review?(wchen)
(Assignee)

Comment 47

3 years ago
Created attachment 8505386 [details] [diff] [review]
Adjusted current node, Java patch, v2
Attachment #8488509 - Attachment is obsolete: true
Attachment #8505386 - Flags: review?(wchen)
(Assignee)

Comment 48

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

Updated

3 years ago
Attachment #8505382 - Flags: review?(wchen) → review+

Updated

3 years ago
Attachment #8505386 - Flags: review?(wchen) → review+

Updated

3 years ago
Attachment #8505377 - Flags: review?(wchen) → review+

Updated

3 years ago
Attachment #8505385 - Flags: review?(wchen) → review+

Updated

3 years ago
Blocks: 1087715
(Assignee)

Comment 49

3 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Keywords: dev-doc-needed

Updated

3 years ago
No longer blocks: 1087715
Duplicate of this bug: 1087715
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.