We need to redraw the slider tick marks when the datalist changes.
Created attachment 148494 [details] Patch
Here is the performance test results for setting id. The variance between runs is too high to tell a difference. https://docs.google.com/spreadsheet/ccc?key=0AoaiM9jljUy8dDZfT3d6YzJ1UFFES2RmbHQ0Skw2bFE
Please upload an EWS-cookable patch. Can you use dom/WebKitMutationObserver.* ?
Created attachment 148769 [details] Patch
Comment on attachment 148769 [details] Patch Attachment 148769 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13025097
Comment on attachment 148769 [details] Patch Attachment 148769 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13033084
Created attachment 148788 [details] Patch
Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13028130
Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13029120
Comment on attachment 148788 [details] Patch Attachment 148788 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13037121
Created attachment 148986 [details] Patch
Comment on attachment 148986 [details] Patch Attachment 148986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13071011 New failing tests: fast/forms/datalist/update-range-with-datalist.html
Created attachment 149000 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 152747 [details] Patch
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13279140
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13276135
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13284127
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13279141
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13276138
Comment on attachment 152747 [details] Patch Attachment 152747 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13274146
Comment on attachment 152747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152747&action=review > Source/WebCore/html/HTMLInputElement.cpp:693 > - } > #if ENABLE(DATALIST) > - else if (attribute.name() == listAttr) > + } else if (attribute.name() == listAttr) { I recommend not to change '}' position. > Source/WebCore/html/HTMLInputElement.cpp:701 > #endif > #if ENABLE(INPUT_SPEECH) > - else if (attribute.name() == webkitspeechAttr) { > + } else if (attribute.name() == webkitspeechAttr) { I recommend move '}' before #endif. > Source/WebCore/html/HTMLInputElement.cpp:1517 > + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); Do we need to create LIstAttributeTargetObserver when the input element is !inDocument()? > Source/WebCore/html/HTMLInputElement.cpp:1523 > + if (isRangeControl()) > + setNeedsStyleRecalc(); Should introduce new virtual member function in InputType, or call setNeedsStyleRecalc() unconditionally. > Source/WebCore/html/HTMLOptionElement.cpp:282 > + ContainerNode* dataList = parentNode(); > + while (dataList && !dataList->hasTagName(datalistTag)) > + dataList = dataList->parentNode(); > + > + if (!dataList) > + return 0; > + > + return static_cast<HTMLDataListElement*>(dataList); Readability of this code isn't good. - Not following an idiom of parent traversal - static_cast<> is far from its requirement (hasTagName(datalistTag)) How about the following? for (ContainerNode parent = parentNode(); parent ; parent = parent->parentNode()) { if (parent->hasTagName(datalistTag)) return static_cast<HTMLDataListElement*>(dataList); } return 0; > LayoutTests/ChangeLog:12 > + * fast/forms/datalist/update-range-with-datalist-expected.txt: Added. > + * fast/forms/datalist/update-range-with-datalist.html: Added. > + * platform/chromium-mac/fast/forms/datalist/update-range-with-datalist-expected.png: Added. This can be a ref-test.
Created attachment 152984 [details] Patch
Comment on attachment 152747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152747&action=review >> Source/WebCore/html/HTMLInputElement.cpp:693 >> + } else if (attribute.name() == listAttr) { > > I recommend not to change '}' position. Done. >> Source/WebCore/html/HTMLInputElement.cpp:701 >> + } else if (attribute.name() == webkitspeechAttr) { > > I recommend move '}' before #endif. Done. >> Source/WebCore/html/HTMLInputElement.cpp:1517 >> + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); > > Do we need to create LIstAttributeTargetObserver when the input element is !inDocument()? No. Added check. >> Source/WebCore/html/HTMLInputElement.cpp:1523 >> + setNeedsStyleRecalc(); > > Should introduce new virtual member function in InputType, or call setNeedsStyleRecalc() unconditionally. We will be doing different things for input type text and color so I added a new virtual member function to InputType. >> Source/WebCore/html/HTMLOptionElement.cpp:282 >> + return static_cast<HTMLDataListElement*>(dataList); > > Readability of this code isn't good. > - Not following an idiom of parent traversal > - static_cast<> is far from its requirement (hasTagName(datalistTag)) > > How about the following? > > for (ContainerNode parent = parentNode(); parent ; parent = parent->parentNode()) { > if (parent->hasTagName(datalistTag)) > return static_cast<HTMLDataListElement*>(dataList); > } > return 0; Done. >> LayoutTests/ChangeLog:12 >> + * platform/chromium-mac/fast/forms/datalist/update-range-with-datalist-expected.png: Added. > > This can be a ref-test. Done.
Comment on attachment 152984 [details] Patch Attachment 152984 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13278488
Comment on attachment 152984 [details] Patch Attachment 152984 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13277512
Comment on attachment 152984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152984&action=review > Source/WebCore/html/HTMLInputElement.cpp:90 > +class ListAttributeTargetObserver : IdTargetObserver { > +public: > + static PassOwnPtr<ListAttributeTargetObserver> create(const AtomicString& id, HTMLInputElement*); > + virtual void idTargetChanged() OVERRIDE; > + > +private: > + ListAttributeTargetObserver(const AtomicString& id, HTMLInputElement*); > + > + HTMLInputElement* m_element; > +}; should be enclosed with #if ENABLE(DATALIST) > Source/WebCore/html/HTMLInputElement.cpp:1522 > +{ > + if (!inDocument()) > + return; > + m_listAttributeTargetObserver = ListAttributeTargetObserver::create(fastGetAttribute(listAttr), this); > +} I know this works well. However this code might make a reader nervous because of possibility of leak of a wrong ListAttributeTargetObserver object. I recommend clearing m_listAttributeTargetObserver if !inDocument(), and use resetListAttributeTargetObserver() in removedFrom() too. > Source/WebCore/html/HTMLInputElement.cpp:1811 > +PassOwnPtr<ListAttributeTargetObserver> ListAttributeTargetObserver::create(const AtomicString& id, HTMLInputElement* element) > +{ > + return adoptPtr(new ListAttributeTargetObserver(id, element)); > +} > + > +ListAttributeTargetObserver::ListAttributeTargetObserver(const AtomicString& id, HTMLInputElement* element) > + : IdTargetObserver(element->treeScope()->idTargetObserverRegistry(), id) > + , m_element(element) > +{ > +} > + > +void ListAttributeTargetObserver::idTargetChanged() > +{ > + m_element->listAttributeTargetChanged(); > +} should be enclosed with #if ENABLE(DATALIST) > Source/WebCore/html/HTMLInputElement.h:398 > + OwnPtr<ListAttributeTargetObserver> m_listAttributeTargetObserver; should be enclosed with #if ENABLE(DATALIST)
Created attachment 153160 [details] Patch
Comment on attachment 153160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153160&action=review > Source/WebCore/html/HTMLInputElement.cpp:1446 > +#if ENABLE(DATALIST) > + resetListAttributeTargetObserver(); > +#endif > HTMLTextFormControlElement::removedFrom(insertionPoint); Node::removedFrom() clears inDocument flag. So this should be HTMLTextFormControlElement::removedFrom(insertionPoint); ASSERT(!inDocument()); #if ENABLE(DATALIST) resetListAttributeTargetObserver(); #endif
Created attachment 153168 [details] Patch
Comment on attachment 153168 [details] Patch ok
Created attachment 153181 [details] Patch
Comment on attachment 153181 [details] Patch Rejecting attachment 153181 [details] from commit-queue. New failing tests: fast/forms/datalist/update-range-with-datalist.html Full output: http://queues.webkit.org/results/13273776
Created attachment 153192 [details] Archive of layout-test-results from gce-cq-03 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: gce-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 153200 [details] Patch
Comment on attachment 153200 [details] Patch Clearing flags on attachment: 153200 Committed r123081: <http://trac.webkit.org/changeset/123081>
All reviewed patches have been landed. Closing bug.