Bug 89544 – Redraw slider tick marks when datalist changes.

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

We need to redraw the slider tick marks when the datalist changes.

[Keishi Hattori]

Created attachment 148494 [details]
Patch

[Keishi Hattori]

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

[Kent Tamura]

Please upload an EWS-cookable patch.

Can you use dom/WebKitMutationObserver.* ?

[Keishi Hattori]

Created attachment 148769 [details]
Patch

[Early Warning System Bot]

Comment on attachment 148769 [details]
Patch

Attachment 148769 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13025097

[Early Warning System Bot]

Comment on attachment 148769 [details]
Patch

Attachment 148769 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13033084

[Keishi Hattori]

Created attachment 148788 [details]
Patch

[Early Warning System Bot]

Comment on attachment 148788 [details]
Patch

Attachment 148788 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13028130

[Early Warning System Bot]

Comment on attachment 148788 [details]
Patch

Attachment 148788 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13029120

[WebKit Review Bot]

Comment on attachment 148788 [details]
Patch

Attachment 148788 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13037121

[Keishi Hattori]

Created attachment 148986 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 152747 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13279140

[Build Bot]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13276135

[Gyuyoung Kim]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13284127

[Build Bot]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13279141

[Early Warning System Bot]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13276138

[Early Warning System Bot]

Comment on attachment 152747 [details]
Patch

Attachment 152747 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13274146

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 152984 [details]
Patch

[Keishi Hattori]

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.

[Build Bot]

Comment on attachment 152984 [details]
Patch

Attachment 152984 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13278488

[Build Bot]

Comment on attachment 152984 [details]
Patch

Attachment 152984 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13277512

[Kent Tamura]

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)

[Keishi Hattori]

Created attachment 153160 [details]
Patch

[Kent Tamura]

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

[Keishi Hattori]

Created attachment 153168 [details]
Patch

[Kent Tamura]

Comment on attachment 153168 [details]
Patch

ok

[Keishi Hattori]

Created attachment 153181 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Keishi Hattori]

Created attachment 153200 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153200 [details]
Patch

Clearing flags on attachment: 153200

Committed r123081: <http://trac.webkit.org/changeset/123081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.