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 document.getElementById #740

Closed
ILyoan opened this issue Aug 19, 2013 · 12 comments
Closed

Implement document.getElementById #740

ILyoan opened this issue Aug 19, 2013 · 12 comments
Labels

Comments

@ILyoan
Copy link
Contributor

@ILyoan ILyoan commented Aug 19, 2013

document.getElementById() needs to be implemented very efficiently. O(1) is is desirable

@jdm
Copy link
Member

@jdm jdm commented Aug 23, 2013

@ILyoan Are you working on this, or is this an open task?

@ILyoan
Copy link
Contributor Author

@ILyoan ILyoan commented Aug 23, 2013

@jdm Not working, just opened.

@jdm
Copy link
Member

@jdm jdm commented Aug 24, 2013

@mihneadb Sounds like you could jump on this if you're interested.

@tetsuharuohzeki
Copy link
Member

@tetsuharuohzeki tetsuharuohzeki commented Aug 24, 2013

May I try to implement this?

@mihneadb
Copy link

@mihneadb mihneadb commented Aug 24, 2013

Sure, go for it. Let me know if you change your mind.

@tetsuharuohzeki
Copy link
Member

@tetsuharuohzeki tetsuharuohzeki commented Aug 24, 2013

Thank you!

2013/8/24 Mihnea Dobrescu-Balaur notifications@github.com

Sure, go for it. Let me know if you change your mind.


Reply to this email directly or view it on GitHubhttps://github.com//issues/740#issuecomment-23201566
.

saneyuki_s
saneyuki.s.snyk@gmail.com

@jdm
Copy link
Member

@jdm jdm commented Aug 31, 2013

This is needed for #841.

@tetsuharuohzeki
Copy link
Member

@tetsuharuohzeki tetsuharuohzeki commented Aug 31, 2013

I think the basic logic of this should be:

  • it would use std::hashmap simply.
  • it need to update hashmap:
    • on instert element subtree to document.
    • on remove element subtree from document.

But I'm worrying about these points.

  • Insertion point of hashmap (I suspect Document would be nice.)
  • Access to Document from insert/remove function.
    • I'm trying it from hubbub_html_parser::parse_html().

How do you think about these?

@jdm
Copy link
Member

@jdm jdm commented Aug 31, 2013

Yes, storing the map in the document sounds fine. I think you might be able to just use the add_to_doc function for updating it on insertion right now, which gets called after parse_html when the whole tree exists.

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Sep 1, 2013

Storing in the document is the way to go.

It's worth thinking about whether to store just the first element with the given ID or all elements (in tree order) with the given ID in the hashmap.

You definitely don't want to do this from the parser; you want to update the map when elements are actually inserted/removed into the document, which can happen without the parser being involved... and conversely the parser can create elements not going into the document (see innerHTML).

@tetsuharuohzeki
Copy link
Member

@tetsuharuohzeki tetsuharuohzeki commented Sep 18, 2013

Hi. My WIP is this stage: tetsuharuohzeki@8685a91
This WIP's next step would be:

  • Observe adding ids via Element::setAttribute().
  • Compatibility with other browsers.

Before go to next step, I have some questions...

This works when elements are appended to/removed from Document.
But this doesn't work that this cannot store id & node pairs to Document.idmap on parse....

To resolve this problem, I doubt that https://github.com/mozilla/servo/blob/d465abdb1c2162ca9eeb72f391ea4a721332500a/src/components/script/script_task.rs#L714 : This document is transmuted from HTMLDocument. It doesn't have any idmap field.

Do you have any good idea about to store id & node pairs to this document?

@jdm
Copy link
Member

@jdm jdm commented Feb 15, 2014

Note: the earlier pull requests implemented the fundamental parts, but we still lack support for multiple elements with the same id.

@Ms2ger Ms2ger closed this Mar 5, 2014
ChrisParis pushed a commit to ChrisParis/servo that referenced this issue Sep 7, 2014
…rse03

additional mathml tests as requested by Denis Ah-Kang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.