Bug 50116

Created attachment 74945 [details] change proposal

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

My comment amounted to resolving the bug as INVALID, in fact. Am I missing something that makes the proposed behavior good for users?

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

Right. My concern is about operating on a selection that may be invisible to the user, or otherwise outside of their mental context.

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

Is this the Firefox behavior on all platforms, or only on Linux? Are there other Windows applications that behave this way?

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

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.

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

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.

Created attachment 75369 [details] Take 2 (updated for style and added a test)

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?

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

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

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

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

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

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 ?

(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

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

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

Sorry, I'm afraid that I'm not following your reasoning.

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

Created attachment 79267 [details] Modified as per Ryosuke's comments

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

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.

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

(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) ?

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

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

(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?

(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. ;)

*** Bug 58647 has been marked as a duplicate of this bug. ***

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