Bug 50116

Summary: On platforms other than Mac, right clicking anywhere discards the selection
Product: WebKit Reporter: Pierre Rossi <pierre.rossi>
Component: HTML EditingAssignee: Pierre Rossi <pierre.rossi>
Status: RESOLVED INVALID    
Severity: Normal CC: ap, avi, benjamin, dglazkov, kalman, rniwa, tonikitoo, victor.fernandez
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 23351    
Bug Blocks:    
Attachments:
Description Flags
change proposal
tonikitoo: review-, tonikitoo: commit-queue-
Take 2 (updated for style and added a test)
ap: review-, ap: commit-queue-
Take 3 (Change windows behavior.)
none
Modified as per Ryosuke's comments rniwa: review-, rniwa: commit-queue-

Description Pierre Rossi 2010-11-26 10:03:01 PST
This doesn't really make sense for right click for instance since one can't really select anything before a context menu pops up, and the editing behavior was fixed so that it doesn't even select the text underneath the cursor on right click now.
Comment 1 Pierre Rossi 2010-11-26 10:07:37 PST
Created attachment 74945 [details]
change proposal
Comment 2 Alexey Proskuryakov 2010-11-26 17:55:20 PST
I'm primarily a Mac user, but I think that having a selection that's far away from the click point (and perhaps even invisible due to scrolling) would be super confusing. You won't know what will be copied when selecting Copy from the context menu.

+    const bool shouldReplaceSelection = (m_frame->editor()->behavior().shouldSelectOnContextualMenuClick()
+                                         || event.event().button() == LeftButton);

This should be on one line.
Comment 3 Antonio Gomes 2010-12-01 06:03:54 PST
Comment on attachment 74945 [details]
change proposal

I looks reasonable, but it needs a layout test.

NOTE: It has to be tested through all editing behaviors code paths: mac, win and unix.

Please also fix what ap commented.

r- due to the lack of tests.
Comment 4 Alexey Proskuryakov 2010-12-01 08:16:54 PST
My comment amounted to resolving the bug as INVALID, in fact. Am I missing something that makes the proposed behavior good for users?
Comment 5 Pierre Rossi 2010-12-01 08:22:11 PST
@Alexey: At the moment, the selection is discarded on right click, it doesn't select anything new. I can't think of any case where that would be much better than leaving it untouched. Besides, this doesn't feel very consistent with other applications' behavior on Linux, where neither gtk nor Qt does this.
Comment 6 Alexey Proskuryakov 2010-12-01 08:29:43 PST
Right. My concern is about operating on a selection that may be invisible to the user, or otherwise outside of their mental context.
Comment 7 Antonio Gomes 2010-12-01 08:30:10 PST
(In reply to comment #5)
> @Alexey: At the moment, the selection is discarded on right click, it doesn't select anything new. I can't think of any case where that would be much better than leaving it untouched. Besides, this doesn't feel very consistent with other applications' behavior on Linux, where neither gtk nor Qt does this.

@ap, pierri: I also agree that this is something we should fix.

For example, in firefox, selection some text with the mouse and right click somewhere else in the page (outside the selection bounds). Firefox shows the context menu, but keeps the previously selected text selected.

We should behavior this way in my opinion on non-Mac editing-behaviors too.
Comment 8 Alexey Proskuryakov 2010-12-01 08:41:08 PST
Is this the Firefox behavior on all platforms, or only on Linux? Are there other Windows applications that behave this way?
Comment 9 Antonio Gomes 2010-12-01 08:48:35 PST
(In reply to comment #8)
> Is this the Firefox behavior on all platforms, or only on Linux? Are there other Windows applications that behave this way?

Hum, good point.

Firefox on linux, scenario 1: 
1) select some text in the page.
2) right click somewhere else
[the original selection if kept]
3) dismiss the context menu by left clicking the elsewhere in the page, outside the selection boundary
[the original selection is kept]

Firefox on linux, scenario 2: 
1) select some text in the page.
2) right click somewhere else
[the original selection if kept]
3) dismiss the context menu by pressing ESC
[the original selection is kept]


Firefox on linux, scenario 1: 
1) select some text in the page.
2) right click somewhere else
[the original selection if kept]
3) dismiss the context menu by left clicking the elsewhere in the page, outside the selection boundary
[the original selection is gone]

Firefox on Windows, scenario 2: 
1) select some text in the page.
2) right click somewhere else
[the original selection if kept]
3) dismiss the context menu by pressing ESC
[the original selection is kept]

I do not have a Mac handy to test though.
Comment 10 Antonio Gomes 2010-12-01 08:50:10 PST
Also in both cases above when there is a text selected, and user right clicks within or beyond the selection boundary, the context menu options that are shown are the same, as if the user had right clicked on the selected text.

It is not what happens in webkit non-mac platform I suppose.
Comment 11 Benjamin Poulain 2010-12-02 05:34:02 PST
(In reply to comment #9)
> I do not have a Mac handy to test though.

I just tested that. Firefox on Mac follows the Linux behavior, the selection is kept when right-clicking outside of the rect.
Comment 12 Benjamin Poulain 2010-12-02 05:46:30 PST
I also tested Windows and the result is curious :)

All the applications I have tried have the same behavior as what Antonio described. Except ... Internet Explorer 9 which as the same behavior as Mac OS.
Comment 13 Pierre Rossi 2010-12-02 05:51:43 PST
Created attachment 75369 [details]
Take 2 (updated for style and added a test)
Comment 14 Antonio Gomes 2010-12-02 07:02:26 PST
Comment on attachment 75369 [details]
Take 2 (updated for style and added a test)

Pierre, thanks for the test case. We would be now covering the ESC case, but I would also like to cover the 'click outside the selection bounds' case. Could you add this other test too, please?
Comment 15 Pierre Rossi 2010-12-02 08:26:32 PST
(In reply to comment #14)
> (From update of attachment 75369 [details])
> Pierre, thanks for the test case. We would be now covering the ESC case, but I would also like to cover the 'click outside the selection bounds' case. Could you add this other test too, please?

Mhh, but I might need a little hint then: how would I take into account the actual platform that the test runs on ? (i.e it has to be ran on windows for the mouse click to actually discard the selection at the same time it discards the menu).
Comment 16 Alexey Proskuryakov 2010-12-02 09:14:06 PST
> I also tested Windows and the result is curious :)
> 
> All the applications I have tried have the same behavior as what Antonio described. Except ... Internet Explorer 9 which as the same behavior as Mac OS.

Could you please list the ones you tried? I'm mostly interested if that included MS Word, Outlook and other major apps known to end users.
Comment 17 Benjamin Poulain 2010-12-06 08:30:18 PST
(In reply to comment #16)
> > I also tested Windows and the result is curious :)
> > 
> > All the applications I have tried have the same behavior as what Antonio described. Except ... Internet Explorer 9 which as the same behavior as Mac OS.
> 
> Could you please list the ones you tried? I'm mostly interested if that included MS Word, Outlook and other major apps known to end users.

I tried with Notepad, wordpad, and all the default utilities I could find on Win7.

On MS word 2007 and Outlook 2007, I get the same behavior as Mac, right click outside the selection discard the selection.
Comment 18 Antonio Gomes 2010-12-06 08:43:00 PST
(In reply to comment #17)
> (In reply to comment #16)
> > > I also tested Windows and the result is curious :)
> > > 
> > > All the applications I have tried have the same behavior as what Antonio described. Except ... Internet Explorer 9 which as the same behavior as Mac OS.
> > 
> > Could you please list the ones you tried? I'm mostly interested if that included MS Word, Outlook and other major apps known to end users.
> 
> I tried with Notepad, wordpad, and all the default utilities I could find on Win7.
> 
> On MS word 2007 and Outlook 2007, I get the same behavior as Mac, right click outside the selection discard the selection.

I think is might make sense here to make not discarding the selection be UNIX specific then, once windows has mixed behavior, and Mac does discards on purpose.
Comment 19 Alexey Proskuryakov 2010-12-06 09:01:01 PST
Comment on attachment 75369 [details]
Take 2 (updated for style and added a test)

r-, because we almost certainly don't want to be different from IE, Outlook and Word on Windows here.

> On MS word 2007 and Outlook 2007, I get the same behavior as Mac, right click
> outside the selection discard the selection.

Do they also select the word under right click, like Mac?

> Mhh, but I might need a little hint then: how would I take into account the actual
> platform that the test runs on ?

The correct way is to test all editing behaviors on all platforms - you can set the behavior with a layoutTestController method.
Comment 20 Antonio Gomes 2010-12-06 09:07:13 PST
> > On MS word 2007 and Outlook 2007, I get the same behavior as Mac, right click
> > outside the selection discard the selection.
> 
> Do they also select the word under right click, like Mac?

no, ap.
Comment 21 Pierre Rossi 2010-12-10 09:33:31 PST
Created attachment 76212 [details]
Take 3 (Change windows behavior.)

@Tonikitoo: well, after a lot of headaches I think I'm going to give up on discarding the menu with the mouse from javascript, since eventSender will pass the event to the page directly and not to X or whatever need to receive it to discard the menu on a given platform. Sure I could do with a middle click or something but that will only do for unstable tests in my opinion, since this seems border-line undefined behavior. Or should I do a Qt/C++ autotest then? What do you think ?
Comment 22 Pierre Rossi 2010-12-10 09:41:13 PST
(In reply to comment #19)
> 
> The correct way is to test all editing behaviors on all platforms - you can set the behavior with a layoutTestController method.

What I meant was that the test would return different results for the windows behavior depending on whether it actually ran on windows or not. 

Due to what I mentioned in my previous comment to Antonio, and the change for the Windows behavior, it probably won't be necessary anymore but what I was looking for was probably something in that fashion:
var onWinPlatform =  navigator.userAgent.search("Windows") != -1
Comment 23 Alexey Proskuryakov 2010-12-10 10:10:10 PST
> What I meant was that the test would return different results for the windows behavior depending on whether it actually ran on windows or not. 

Why? Actual behavior should respect editing behavior preference.
Comment 24 Pierre Rossi 2010-12-16 13:03:39 PST
(In reply to comment #23)
> > What I meant was that the test would return different results for the windows behavior depending on whether it actually ran on windows or not. 
> 
> Why? Actual behavior should respect editing behavior preference.

The fact that the selection was discarded when clicking outside of the context menu was not caused by anything else than the fact that while this click discards the menu, it's *also* passed to the webpage, and this is because of the way events work on windows. 

Now the selection is discarded with the right click, to follow IE9 behavior, so it's not a problem anymore.
Comment 25 Alexey Proskuryakov 2010-12-16 13:23:10 PST
Sorry, I'm afraid that I'm not following your reasoning.
Comment 26 Ryosuke Niwa 2010-12-17 11:34:51 PST
Comment on attachment 76212 [details]
Take 3 (Change windows behavior.)

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

> WebCore/editing/EditingBehavior.h:64
> +    // On Linux, right clicking outside of the current selection shouldn't discard the existing selection
> +    bool shouldDiscardCurrentSelectionOnRightClick() const { return m_type != EditingUnixBehavior; }

Nit: since editing behavior is "unix", we should probably say "On UNIX, right clicking..."

> WebCore/page/EventHandler.cpp:403
> +    // Don't alter the selection for buttons other than the left button unless it's desired behavior

Nit: I don't think this comment is necessary.  It repeats what code says.

> WebCore/page/EventHandler.cpp:405
> +    if (m_frame->selection()->shouldChangeSelection(newSelection) && shouldReplaceSelection)

I'm not sure if we want to call editing delegates if we're not changing the selection.  It's odd to call editing delegates to ask if the selection should change and then not changing regardless of what the delegates returns.  We should probably swap the order and do: shouldReplaceSelection && m_frame->selection()->shouldChangeSelection(newSelection).
Comment 27 Pierre Rossi 2011-01-18 05:30:03 PST
Created attachment 79267 [details]
Modified as per Ryosuke's comments
Comment 28 Antonio Gomes 2011-01-21 21:07:26 PST
Comment on attachment 79267 [details]
Modified as per Ryosuke's comments

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

I have a question: with the patch applied, context menu does not deselect the text on UNIX editing behavior, but what options does the context menu show? Are they relative to selected content (i.e. copy, cut, etc) or relative to the page (reload, back, forward, etc). Firefox on linux does the former to me...

> LayoutTests/editing/selection/selection-modified-on-right-click.html:21
> +

Unneeded extra line
Comment 29 Ryosuke Niwa 2011-02-17 00:11:55 PST
Pierre, can you answer Antonio's question so that I can proceed with my review?

Also, hit testing belongs HTML Editing component, not Text component.
Comment 30 Pierre Rossi 2011-02-17 14:12:47 PST
(In reply to comment #28)
> (From update of attachment 79267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79267&action=review
> 
> I have a question: with the patch applied, context menu does not deselect the text on UNIX editing behavior, but what options does the context menu show? Are they relative to selected content (i.e. copy, cut, etc) or relative to the page (reload, back, forward, etc). Firefox on linux does the former to me...
> 
> > LayoutTests/editing/selection/selection-modified-on-right-click.html:21
> > +
> 
> Unneeded extra line

With the patch applied, the contents of the context menu depends on whether the click happened within the selected range or not (so the behavior is pretty much untouched in that respect I would say). So by default (in QtTestBrowser) if I click within the selection it offers "Copy", else only "Reload"

Actually now that you point it out, firefox always offer a Copy action as long as there's a selection for me on Linux.
Comment 31 Pierre Rossi 2011-02-17 14:15:11 PST
(In reply to comment #29)
> Pierre, can you answer Antonio's question so that I can proceed with my review?
> 

Thanks for the reminder, I had completely forgotten to answer this comment.

> Also, hit testing belongs HTML Editing component, not Text component.

I'm afraid I don't follow ? Could you clarify what this means practically (I suspect I have to move the layout test somewhere else but I have no clue where) ?
Comment 32 Ryosuke Niwa 2011-02-17 16:49:32 PST
(In reply to comment #31)
> > Also, hit testing belongs HTML Editing component, not Text component.
> 
> I'm afraid I don't follow ? Could you clarify what this means practically (I suspect I have to move the layout test somewhere else but I have no clue where) ?

Oh, I just changed the component of this bug from Text to HTML Editing.  That's all.  Your test is already in LayoutTests/editing so it's fine.
Comment 33 Ryosuke Niwa 2011-02-17 17:26:40 PST
Comment on attachment 79267 [details]
Modified as per Ryosuke's comments

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.
Comment 34 Pierre Rossi 2011-02-19 12:09:43 PST
(In reply to comment #33)

> > 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.

I don't like this idea that much, as it would mean the context menu pops out as the selection gets modified, this feels confusing to me (yes, apparently that's the way it works on Mac...)
Comment 35 Ryosuke Niwa 2011-02-21 01:31:52 PST
(In reply to comment #34)
> (In reply to comment #33)
> > 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.
> 
> I don't like this idea that much, as it would mean the context menu pops out as the selection gets modified, this feels confusing to me (yes, apparently that's the way it works on Mac...)

Is this what users expect on all Unix platforms?
Comment 36 Pierre Rossi 2011-02-22 03:43:11 PST
(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > 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.
> > 
> > I don't like this idea that much, as it would mean the context menu pops out as the selection gets modified, this feels confusing to me (yes, apparently that's the way it works on Mac...)
> 
> Is this what users expect on all Unix platforms?

I don't know really, it's just the behavior I get in Firefox, and I feel it makes sense, or at least it's something I can live with. 
If you have any usability gurus at hand to discuss this, that'd be nice. ;)
Comment 37 Benjamin Poulain 2011-04-15 03:54:17 PDT
*** Bug 58647 has been marked as a duplicate of this bug. ***
Comment 38 Jocelyn Turcotte 2014-02-03 03:50:50 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.