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

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: hsivonen)

Tracking

({dev-doc-needed})

Trunk
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

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
Posted file testcase (obsolete) —
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.
Attachment #766763 - Attachment is obsolete: true
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
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.
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
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
Duplicate of this bug: 1034495
(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)
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8474224 - Flags: review?(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.
Attachment #8474224 - Flags: review?(hsivonen) → review-
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
(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)
Attachment #8474226 - Attachment is obsolete: true
Attachment #8481788 - Flags: review?(hsivonen)
Posted patch mozilla patch (obsolete) — Splinter Review
Attachment #8474224 - Attachment is obsolete: true
Attachment #8481789 - Flags: review?(hsivonen)
(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.)
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
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.
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.
Another update: It seems that the attached patch doesn't work right with foreignObject context. I'm working on a new patch.
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-
Attachment #8481789 - Flags: review?(hsivonen) → review-
(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
Attachment #8481788 - Attachment is obsolete: true
Attachment #8481789 - Attachment is obsolete: true
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 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)
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)
Attachment #8488510 - Flags: review?(wchen)
Attachment #8488516 - Flags: review?(wchen)
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+
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-
Attachment #8488516 - Flags: review?(wchen) → review+
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
(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+
Attachment #8488510 - Attachment is obsolete: true
Attachment #8505385 - Flags: review?(wchen)
Attachment #8488509 - Attachment is obsolete: true
Attachment #8505386 - Flags: review?(wchen)
Attachment #8505382 - Flags: review?(wchen) → review+
Attachment #8505386 - Flags: review?(wchen) → review+
Attachment #8505377 - Flags: review?(wchen) → review+
Attachment #8505385 - Flags: review?(wchen) → review+
Blocks: 1087715
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.