Bug 31428

Summary:

getElementsByTagName("div")['elementId'] lookup fails if two elements have the same id

Product:

WebKit

Reporter:

Chang Shu <cshu>

Component:

DOM

Assignee:

Chang Shu <cshu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

5578

Attachments:

Description	Flags
fix patch	eric: review-
update test case based on Eric's review	eric: review-
patch 3	none

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-	Details Formatted Diff Diff
update test case based on Eric's review (3.37 KB, patch) 2009-11-25 12:04 PST, Chang Shu	eric: review-	Details Formatted Diff Diff
patch 3 (3.37 KB, patch) 2009-11-29 15:53 PST, Chang Shu	no flags	Details Formatted Diff Diff
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.