RESOLVED FIXED 31428
getElementsByTagName("div")['elementId'] lookup fails if two elements have the same id 
https://bugs.webkit.org/show_bug.cgi?id=31428
Summary getElementsByTagName("div")['elementId'] lookup fails if two elements have th...
Chang Shu
Reported 2009-11-12 12:39:18 PST
The following test script(partial) failed to work as expected. Other major browsers passed the test. <body> <div name='elem1' id='attr_value11' attr_name12='attr_value12'/> <p name='elem4' id='attr_value21' attr_name42='attr_value42'/> <div name='elem2' id='attr_value21' attr_name22='attr_value22'/> <script> elems = document.getElementsByTagName('div'); elem = elems['attr_value21']; shouldBe('elem.attributes[0].value', '"elem2"'); </script> The full test case will be added to LayoutTests as fast/dom/Element/id-in-node-list-index01.html
Attachments
fix patch (3.53 KB, patch)
2009-11-12 13:06 PST, Chang Shu 		eric: review-
DetailsFormatted DiffDiff
update test case based on Eric's review (3.37 KB, patch)
2009-11-25 12:04 PST, Chang Shu 		eric: review-
DetailsFormatted DiffDiff
patch 3 (3.37 KB, patch)
2009-11-29 15:53 PST, Chang Shu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chang Shu
Comment 1 2009-11-12 13:06:56 PST
Created attachment 43091 [details] fix patch
Eric Seidel (no email)
Comment 2 2009-11-13 13:40:51 PST
I do not understand the patch or the test case. Can you please explain better what you're trying to do here?
Chang Shu
Comment 3 2009-11-13 13:59:35 PST
(In reply to comment #2) > I do not understand the patch or the test case. Can you please explain better > what you're trying to do here? Sure. In the test case, I have 3 elements: 2 are <div> and 1 is <p>. The call to document.getElementsByTagName('div') should return the 2 <div> elements. Then using Id='attr_value21' as index should return elem2. The <p> element happens to have the same Id as elem2 and this causes trouble because the optimized code(see below) assumes the uniqueness of Id. Specifically, the "node" returned from getElementById is expected to be elem2, however, it is actually elem4, the <p> element. And the function returns 0. The 2nd part of the following function should always work, though. So in case "node" is not 0, my patch continue on the 2nd part. As the case is rare that a document has duplicated Ids, the performance should be ok. Node* DynamicNodeList::itemWithName(const AtomicString& elementId) const { if (m_rootNode->isDocumentNode() || m_rootNode->inDocument()) { Element* node = m_rootNode->document()->getElementById(elementId); if (node && nodeMatches(node)) { for (Node* p = node->parentNode(); p; p = p->parentNode()) { if (p == m_rootNode) return node; } } return 0; } unsigned length = this->length(); for (unsigned i = 0; i < length; i++) { Node* node = item(i); if (node->isElementNode() && static_cast<Element*>(node)->getIDAttribute() == elementId) return node; } return 0; }
Eric Seidel (no email)
Comment 4 2009-11-25 08:53:21 PST
Comment on attachment 43091 [details] fix patch cshu and I spoke over IRC. The fix looks correct, but we need to better communicate here why it's correct. 1. The test case needs to be cleaned up to use better variable naming, and probably just shorter in general. We could consider converting it to a normal "script-test", although that's not required. http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree 2. We need information on why this change is correct. Does this new behavior conform to a spec? Does this new behavior match FF/IE? If the answer to either of those questions is "no" we need to know why. Basically we just need to make this patch super-easy to understand, and thus review. Once that's done, I think this will be a very quick r+.
Chang Shu
Comment 5 2009-11-25 12:03:05 PST
(In reply to comment #4) > (From update of attachment 43091 [details]) > cshu and I spoke over IRC. The fix looks correct, but we need to better > communicate here why it's correct. > > 1. The test case needs to be cleaned up to use better variable naming, and > probably just shorter in general. We could consider converting it to a normal > "script-test", although that's not required. > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree Please review the 2nd patch with test case updates. > 2. We need information on why this change is correct. Does this new behavior > conform to a spec? Does this new behavior match FF/IE? If the answer to > either of those questions is "no" we need to know why. 1. In the test case, the id is unique in the retrieved node list. The new behavior returns the correct node while the current behavior returns null. So the new behavior should conform to the spec. 2. The new behavior matches both Firefox 3.5.5 and IE8.
Chang Shu
Comment 6 2009-11-25 12:04:31 PST
Created attachment 43862 [details] update test case based on Eric's review
Eric Seidel (no email)
Comment 7 2009-11-29 14:03:54 PST
Comment on attachment 43862 [details] update test case based on Eric's review I don't see why you use the global variable 'elem': var elem = elems['id1']; 20 shouldBe('elem.getAttribute("name")', '"name1"'); would be more clear as: shouldBe('elems["id2"].getAttribute("name")', '"name1"'); The old behavior was to return null from elems['id1'] correct? generally we try to make comments into complete sentences with periods: 134 // in case multiple nodes with same name, fall through // In the case of multiple nodes with the same name, just fall through. Your ChangeLog comment isn't quite english. :) Understandable as I'm assuming english isn't your mother tongue. 5 Continue search for matching node in case multiple nodes with 6 the same Id exist. Continue to search for matching nodes in the case where multiple nodes have the same id. I think we should go one more round. In general this looks great!
Chang Shu
Comment 8 2009-11-29 15:53:09 PST
Created attachment 43995 [details] patch 3
Chang Shu
Comment 9 2009-11-29 16:02:23 PST
(In reply to comment #7) > (From update of attachment 43862 [details]) > I don't see why you use the global variable 'elem': > var elem = elems['id1']; > 20 shouldBe('elem.getAttribute("name")', '"name1"'); > would be more clear as: > shouldBe('elems["id2"].getAttribute("name")', '"name1"'); > The old behavior was to return null from elems['id1'] correct? elems["id1"] works fine before and after my change. elems["id2"] failed to work before as both elem2 and elem3 have the same id, "id2". Thanks for correcting my English, too. :) I guess it's good I can improve both my technical skills and my English during this review process.
Eric Seidel (no email)
Comment 10 2009-11-29 16:51:29 PST
Comment on attachment 43995 [details] patch 3 YAY! Thanks for all your patience!
WebKit Commit Bot
Comment 11 2009-11-29 17:02:51 PST
Comment on attachment 43995 [details] patch 3 Clearing flags on attachment: 43995 Committed r51471: <http://trac.webkit.org/changeset/51471>
WebKit Commit Bot
Comment 12 2009-11-29 17:02:56 PST
All reviewed patches have been landed. Closing bug.
Chang Shu
Comment 13 2009-11-29 17:44:44 PST
(In reply to comment #10) > (From update of attachment 43995 [details]) > YAY! Thanks for all your patience! Eric, thanks very much for reviewing my patch on Sunday night!
Note You need to log in before you can comment on or make changes to this bug.