Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Element.innerHTML setter #849

Closed
jdm opened this issue Aug 31, 2013 · 17 comments
Closed

Implement Element.innerHTML setter #849

jdm opened this issue Aug 31, 2013 · 17 comments
Labels
A-content/dom Interacting with the DOM from web content

Comments

@jdm
Copy link
Member

jdm commented Aug 31, 2013

Needed for #841.

@jdm
Copy link
Member Author

jdm commented Nov 11, 2013

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 11, 2013

@jdm
Copy link
Member Author

jdm commented Nov 12, 2013

For the setter, we should make hubbub_html_parser::parse_html (http://mxr.mozilla.org/servo/source/src/components/script/html/hubbub_html_parser.rs#248) take a root node argument instead of creating an HTMLHtmlElement node by default.

@tetsuharuohzeki
Copy link
Contributor

Does anyone start to work this?

@jdm
Copy link
Member Author

jdm commented Nov 12, 2013

Yes, @jaeminMoon is working on it.

@bzbarsky
Copy link
Contributor

It's not just taking a root node argument. To get correct behavior you have to set up the parsing context based on the target node, then parse into a DocumentFragment, then replace the node's kids with the fragment.

In particular, parsing into the DocumentFragment can throw, and in that case the node's kids should not be affected, which is pretty hard to do if you're parsing directly into the node.

@jdm
Copy link
Member Author

jdm commented Nov 12, 2013

Is "parsing context" a spec concept I should be familiar with?

@jdm jdm changed the title Implement Element.innerHTML Implement Element.innerHTML setter Apr 18, 2014
@jdm jdm added the C-assigned label Jun 2, 2014
@jdm
Copy link
Member Author

jdm commented Jun 2, 2014

@Adenilson is working on this.

@Adenilson
Copy link
Contributor

So the game plan: refactor parse_html() to be more modular and decoupled, targeting to reuse the code while receiving a DOMString in innerHTML. Restrictions: allow the code to be upgraded when the new HTML parser lands.

There are really 3 steps on it:
a) Move parser creation to its own function.
b) Do the same for the TreeHandler.
c) Use this in innerHTML (as a new exported API in element.rs/Element.webidl).

Manishearth added a commit that referenced this issue Jul 22, 2014
Move Parser creation to its own function (issue #849).
ChrisParis pushed a commit to ChrisParis/servo that referenced this issue Sep 7, 2014
…SourceBuffer-abort

Tests for SourceBuffer#abort()
@jdm
Copy link
Member Author

jdm commented Sep 12, 2014

Further notes for the benefits of NCSU students who may be working on this (@Adenilson, please talk to me before continuing to work on this):

  • make parse_html take a Node instead of Document; modify the code that requires a Document to use a dynamic cast (DocumentCast::to_ref)
  • make the innerHTML setter create a new DocumentFragment
  • prepend the setter value with <[tagName] id="___secret_servo_id___"> and append </tagName>
  • call parse_html on the modified setter value with the new DocumentFragment
  • remove all existing children of the current element
  • find the element with the special id in the children of the DocumentFragment and adopt all child nodes into the containing element on which innerHTML was set
  • use the result of parse_html to fetch any new JS scripts and execute them

(note, adapted from http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#html-fragment-parsing-algorithm)

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 13, 2014

I'm not convinced this won't require a lot of rewriting when we land the new HTML parser.

@Adenilson
Copy link
Contributor

@jdm Sure, when I resume work on this, I will be coordinating with you guys.

@Ms2ger yes and no... Let me explain it better. I didn't pursued this in part because I was hoping that the new parser would land soon. That being said, the new parser will need to support the user case i.e. receive a Node (as also a Document), and by having the interfaces available would be a matter of swapping the Hubbub based parser for the new one.

@kmcallister
Copy link
Contributor

The new parser will land shortly after the next Rust upgrade. It's designed to handle fragment parsing, but there are a few pieces missing; see kmcallister/html5ever#16.

@jdm jdm modified the milestones: mile, Dogfooding Nov 6, 2014
@jdm
Copy link
Member Author

jdm commented Dec 17, 2014

@eddyb Is this something you were seriously looking into? I know @cparis expressed interest in working on it, and it's quite high on our list of priorities right now.

@jdm jdm removed the C-assigned There is someone working on resolving the issue label Dec 17, 2014
@eddyb
Copy link
Contributor

eddyb commented Dec 17, 2014

Well, I do have partial fragment parsing implemented right now, if you want me to publish that. Needs a few changes to html5ever though, so I would prefer to do it immediately after a rustup, unless someone wants to help me with backporting changes, or applying them directly to servo's branch.
The big thing missing is determining the initial insertion mode from the context, but if the context is made of only the outer element's tag, then it's likely just a couple more lines in html5ever.

Well, I've decided to push what I have so far (cc @cparis): html5ever and servo.

@kmcallister
Copy link
Contributor

Needs a few changes to html5ever though, so I would prefer to do it immediately after a rustup, unless someone wants to help me with backporting changes

Ah, I see your changes to html5ever are quite small. You can go ahead and submit that as a PR against master. Then we'll reset branch servo to master, and revert rustup patches from master as necessary.

bors-servo pushed a commit that referenced this issue Mar 18, 2015
This addresses #849.  This PR cannot land until the corresponding PR (servo/html5ever#91) in html5ever lands. I've done some simple testing of this code, but I don't consider it thorougly tested yet. I wanted to start getting feedback about the overall design before I spend more time polishing the details, and testing.
@kmcallister
Copy link
Contributor

This is fixed by #4888 \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content
Projects
None yet
Development

No branches or pull requests

7 participants