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
Created attachment 43091 [details] fix patch
I do not understand the patch or the test case. Can you please explain better what you're trying to do here?
(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; }
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+.
(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.
Created attachment 43862 [details] update test case based on Eric's review
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!
Created attachment 43995 [details] patch 3
(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.
Comment on attachment 43995 [details] patch 3 YAY! Thanks for all your patience!
Comment on attachment 43995 [details] patch 3 Clearing flags on attachment: 43995 Committed r51471: <http://trac.webkit.org/changeset/51471>
All reviewed patches have been landed. Closing bug.
(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!