Bug 46950 – Some bugs of search cancel button and spin button about state change in an event handler

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Search field cancel button keeps event capturing

[Kent Tamura]

SearchFieldCancelButtonElement::defaultEventHandler and SpinButtonElement::defaultEventHandler calls focus(), which can change the element type, value, etc.

[Kent Tamura]

Created attachment 69417 [details]
Patch

[Darin Adler]

Comment on attachment 69417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69417&action=review

I’m going to say review+, but I think this can be improved and I’d be willing to review again if you post an improved version.

> WebCore/rendering/TextControlInnerElements.cpp:222
> +        RefPtr<HTMLInputElement> protector(input);

Why not just make the local variable “input” be a RefPtr in the first place? The protector idiom should only be used when there’s no better option, and I think making the pointer be a RefPtr from the beginning is better.

> WebCore/rendering/TextControlInnerElements.cpp:227
> +        // By an event handler, the input element might be removed or hidden,
> +        // the cancel button might be hidden, or the input type might be changed.

The grammar here is not great.

You could say: “An event handler might remove or hide the input element, hide the cancel button, or change the input type.”

But it would be even better to make it clearer what you mean by event handler. You are alluding to the fact that focus and select calls can invoke JavaScript, but this may not be clear to the person reading the code.

So you could say: “Focusing and selecting the field may run JavaScript in response to the event. That code might remove or hide the input element, hide the cancel button, or change the input type.”

But I’m not sure that list of specific the code can do is all that helpful, nor is the comment pointing clearly enough to the part of this function that was written to deal with those possibilities. I think the point here is that we protect “this” so that we can call renderer() and get access to m_capturing, and protect “input” so that we can call the select function even after calling the focus function.

> WebCore/rendering/TextControlInnerElements.cpp:276
> +void SpinButtonElement::detach()

Does the test case cover this? To be specific: Does the test fail if you remove this code?

> WebCore/rendering/TextControlInnerElements.cpp:314
>              RefPtr<Node> protector(input);

Why not just make the local variable “input” be a RefPtr in the first place? The protector idiom should only be used when there’s no better option, and I think making the pointer be a RefPtr from the beginning is better.

> WebCore/rendering/TextControlInnerElements.cpp:318
> +            // Input type might be changed by an event handler. This shadow node might be detached.

Again, I think a much better comment can be written.

[Kent Tamura]

Created attachment 73039 [details]
Patch 2

[Kent Tamura]

Comment on attachment 69417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69417&action=review

>> WebCore/rendering/TextControlInnerElements.cpp:222
>> +        RefPtr<HTMLInputElement> protector(input);
> 
> Why not just make the local variable “input” be a RefPtr in the first place? The protector idiom should only be used when there’s no better option, and I think making the pointer be a RefPtr from the beginning is better.

Oh, right.  "input' should be a RefPtr.  It's simpler.

>> WebCore/rendering/TextControlInnerElements.cpp:227
>> +        // the cancel button might be hidden, or the input type might be changed.
> 
> The grammar here is not great.
> 
> You could say: “An event handler might remove or hide the input element, hide the cancel button, or change the input type.”
> 
> But it would be even better to make it clearer what you mean by event handler. You are alluding to the fact that focus and select calls can invoke JavaScript, but this may not be clear to the person reading the code.
> 
> So you could say: “Focusing and selecting the field may run JavaScript in response to the event. That code might remove or hide the input element, hide the cancel button, or change the input type.”
> 
> But I’m not sure that list of specific the code can do is all that helpful, nor is the comment pointing clearly enough to the part of this function that was written to deal with those possibilities. I think the point here is that we protect “this” so that we can call renderer() and get access to m_capturing, and protect “input” so that we can call the select function even after calling the focus function.

I changed a way to fix the issue.
I moved focus() and select() because I wanted to make sure the cancel button was visible before starting event capturing.
The updated patch removes the visibility check in mouseup event, and doesn't change the mousedown event handling.  It is much simpler.
I removed the comment.

>> WebCore/rendering/TextControlInnerElements.cpp:276
>> +void SpinButtonElement::detach()
> 
> Does the test case cover this? To be specific: Does the test fail if you remove this code?

Ah, no.  I have added a test case for this.

>> WebCore/rendering/TextControlInnerElements.cpp:318
>> +            // Input type might be changed by an event handler. This shadow node might be detached.
> 
> Again, I think a much better comment can be written.

I have improved the comment.

[Kent Tamura]

Created attachment 73041 [details]
Patch 3

[Kent Tamura]

Created attachment 76495 [details]
Patch 4 (rebased)

[Kent Tamura]

Created attachment 79903 [details]
Patch 5

rebased

[Dimitri Glazkov (Google)]

Comment on attachment 79903 [details]
Patch 5

ok

[Kent Tamura]

Comment on attachment 79903 [details]
Patch 5

Clearing flags on attachment: 79903

Committed r76566: <http://trac.webkit.org/changeset/76566>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.