[webkit-reviews] review denied: [Bug 50116] On platforms other than Mac, right clicking anywhere discards the selection : [Attachment 79267] Modified as per Ryosuke's comments

Thu Feb 17 17:26:40 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 50116: On platforms other than Mac, right clicking anywhere discards the
selection
https://bugs.webkit.org/show_bug.cgi?id=50116

Attachment 79267: Modified as per Ryosuke's comments
https://bugs.webkit.org/attachment.cgi?id=79267&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79267&action=review

> LayoutTests/editing/selection/selection-modified-on-right-click.html:1
> +<body onload="load()">

Missing <!DOCTYPE html>.  Do you really need to wait until page load?  I feel
like you can just run the test in the script element?

> WebCore/ChangeLog:5
> +	   https://bugs.webkit.org/show_bug.cgi?id=50116

Missing bug title.

> WebCore/page/EventHandler.cpp:404
>      } else
>	   newSelection = VisibleSelection(visiblePos);
> -    
> -    if (m_frame->selection()->shouldChangeSelection(newSelection))
> +
> +    const bool shouldReplaceSelection =
(m_frame->editor()->behavior().shouldDiscardCurrentSelectionOnRightClick() ||
event.event().button() == LeftButton);
> +    if (shouldReplaceSelection &&
m_frame->selection()->shouldChangeSelection(newSelection))

It seems like we should avoid setting selection only if the above else
statement was executed.  i.e. we should set selection when selection is
extended even if the editing behavior was Linux, and the right button is
pressed.