[Webkit-unassigned] [Bug 70496] Cannot select multiple options by mouse dragging in <select multiple="multiple" size="7"> list

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70496


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114266|review?                     |review-
               Flag|                            |




--- Comment #3 from Kent Tamura <tkent at chromium.org>  2011-11-09 20:44:46 PST ---
(From update of attachment 114266)
View in context: https://bugs.webkit.org/attachment.cgi?id=114266&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:1208
> +    } else if (event->type() == eventNames().mousemoveEvent && event->isMouseEvent()
> +                  && static_cast<MouseEvent*>(event)->button() == LeftButton && static_cast<MouseEvent*>(event)->buttonDown()
> +                  && !static_cast<MouseEvent*>(event)->ctrlKey() && !toRenderBox(renderer())->canBeScrolledAndHasScrollableArea()) {

* The indentation looks wrong for folded lines. You don't need to fold a long lines.

* You don't need to check these conditions in one 'if' statement. You can write:

} else if (event->type() == evnetNames().mousemoveEvent && event->isMouseEvent()) {
    MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
    If (mouseEvent->button() != LeftButton || !mouseEvent->buttonDown())
        return;
    ....

* Checking only ctrlKey() is wrong. It should be metaKey() in Mac.  Please refeer to mousedown handling in this function.

* Do we need to check !canBeScrolledAndHasScrollableArea()?

> Source/WebCore/html/HTMLSelectElement.cpp:1209
> +        // Convert to coords relative to the list box if needed.

This comment is not helpful.  Please remove it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.