When one modify the content of an element dynamically using either DOM functions or innerHTML property (via javascript), the element content is acceded correctly the first time the script run. But on the second run, Safari fail to access the (previously) modified content and report it as being Null. Meaning its not possible to modify again the same element's content. see url provided for details of the script and live exemple. The problem occure on Safari 1.2 and 1.3 (not tested on 2.0, as i dont have it). Script work fine with: [win] Mozilla/Firefox/Opera/IE6 [linux] Firefox/Konkeror/Epiphany [mac] Firefox/NS7
Bug confirmed, it does work in firefox.
Created attachment 2610 [details] Improve handling of duplicate ids Turns out that the problem is because the document contains two elements with the same id for a short while. When one element is removed, the id table is not being updated. Here's a patch that fixes it. I'm a little concerned with always inserting a count value of 1 in the id count table, but that shouldn't be too much overhead.
Comment on attachment 2610 [details] Improve handling of duplicate ids The usual way to handle something where there's a cache is to make the member mutable rather than making the method non-const. I suggest putting mutable in from of the m_idCount declaration, and leaving the methods const. In DocumentImpl::getElementById, you should use qId when calling find on m_elementsById. The code that checks the ID of an element should call hasID before calling getAttribute(ATTR_ID); it's more efficient that way since most elements don't have IDs. I think it's a little strange to use a QDict<int>, but actually store the integers as pointers. Maybe QDict<void> would be a better way to declare it for now. And later we should change it to an actual integer dictionary. This: + int idCount; + if ((idCount = (int)m_idCount.find(qId))) { would be better written like this: if (int idCount = (int)m_idCount.find(qId)) { But instead, I think the code should assert that an idCount is found. If it's not, we have an internal inconsistency error and I'd like to drop into the debugger. It's probably best to put the remove code in an "if (idCount <= 1)" and then assert that the count is 1 in that branch. Or something similar and equivalent. Glad you used the strategy I suggested to fix the bug; I think this is simple and will be nice and efficient. One further idea about how to make m_idCount smaller that would make the code only slightly more complex: The m_idCount dictionary could hold counts that do not include the one in m_elementsById. Thus we'd only bump the count by 1 if there was already an element in m_elementsById, and we'd only decrement if it's an element other than the one in m_elementsById being removed. Also, when an element was found and put into m_elementsById, we would decrement the count. The benefit here is that the m_idCount table would then be empty almost all the time; we could even have the member variable be a pointer and allocate it only when needed. Documents without duplicate IDs would have less overhead. Just an idea -- the existing code is fine too.
Created attachment 2616 [details] Address comments I implemented your idea of making the count table smaller. It's not possible to use a void dict since the QDict destructor (optionally) deletes the pointer, and you can't delete a void pointer. Maciej told me he used a char QDict for a similar integer-in-pointer hack so I did the same.
Comment on attachment 2616 [details] Address comments The code in DocumentImpl::getElementById says "(char *)idCount - 1" in a place where it should say "(char *)(idCount - 1)". That will work, but only because pointer arithmetic and integer arithmetic happen to be the same in this case. I'm going to mark this r=me, although I think we should make that change. I wondered at first whether m_accessKeyDictValid needed to be set in the getElementById function. Then I realized that I don't understand the code setting m_accessKeyDictValid at all. Why on earth does the presence or absence of an ID have anything to do with the validity of the access key dictionary? Clearly this patch makes things no worse in that respect, but I'm pretty sure this points to a bug in the access key support.
Comment on attachment 2616 [details] Address comments I just realized that this bug does not have a suitable layout test. The bug fix is great, but I'm going to have to mark this "review-" until we have a layout test. I though the original test.html could be used as a layout test, but I was mistaken -- it's too complicated, and requires user input.
(In reply to comment #6) > (From update of attachment 2616 [details] [edit]) > I just realized that this bug does not have a suitable layout test. The bug fix > is great, but I'm going to have to mark this "review-" until we have a layout > test. I though the original test.html could be used as a layout test, but I was > mistaken -- it's too complicated, and requires user input. > + layout-tests/fast/dom/ids/duplicate-ids-expected.txt: Added. + layout-tests/fast/dom/ids/duplicate-ids.html: Added. + layout-tests/fast/dom/ids/switch-children-expected.txt: Added. + layout-tests/fast/dom/ids/switch-children.html: Added. These are in the patch. I guess they can be improved to specify what's being tested and the expected outcome.
Created attachment 2923 [details] Improve test case Here's a new patch with an improved test case.
Comment on attachment 2923 [details] Improve test case r=me since Darin already reviewed and now there is a test case.
it seems the bug still exist on safari Build 312.3.1, is that normal ? Will the bug be fixed in a futur released version?
Anders is a commiter, so I'll leave this one for him to commit.
This was committed to ToT a long time ago; closing.
but the bug still exist on safari Build 312.3.1, is that really normal ?
Yes; AFAIK it's up to Apple to merge the changes back to previous versions of WebKit
who should i bug for that ?