Bug 23351 – Add preference to not select when right-clicked

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

On some problems, it may be necessary to not select content on right-click.

Currently, the implementation uses PLATFORM defines, but ideally it should be a preference.

See discussion here: https://bugs.webkit.org/show_bug.cgi?id=15279

[Ojan Vafai]

This should just use the existing EditingBehavior preference:

Settings.h:
enum EditingBehavior { EditingMacBehavior, EditingWindowsBehavior };

There's a bug filed to add EditingLinuxBehavior as well, though I can't seem to find it. In either case, this is a case where we should select the word on right-click iff the editing behavior is EditingMacBehavior.

[Antonio Gomes]

(In reply to comment #1)
> This should just use the existing EditingBehavior preference:
> 
> Settings.h:
> enum EditingBehavior { EditingMacBehavior, EditingWindowsBehavior };
> 
> There's a bug filed to add EditingLinuxBehavior as well, though I can't seem to
> find it. In either case, this is a case where we should select the word on
> right-click iff the editing behavior is EditingMacBehavior.

https://bugs.webkit.org/show_bug.cgi?id=36627 much probably

[Avi Drissman]

Created attachment 68458 [details]
Switch based on pref, not compile flag

[Avi Drissman]

Causes Chromium issues:

http://code.google.com/p/chromium/issues/detail?id=53572
http://code.google.com/p/chromium/issues/detail?id=56371

[WebKit Review Bot]

Attachment 68458 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/page/EventHandler.cpp:2009:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Avi Drissman]

Created attachment 68460 [details]
Style fix

[Tony Chang]

This seems fine to me, but can someone working on QT verify that this is something the behavior they want?  Antonio?

[Antonio Gomes]

(In reply to comment #7)
> This seems fine to me, but can someone working on QT verify that this is something the behavior they want?  Antonio?

It seems fine. I will review that in one hour or so.

[Ojan Vafai]

Comment on attachment 68460 [details]
Style fix

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

> WebCore/editing/EditingBehavior.h:61
> +    // On Mac, when processing a contextual click, the object being clicked upon should be selected.
> +    bool shouldSelectOnContextualMenuClick() const { return m_type == EditingMacBehavior; }

Naming nit: shouldSelectWordOnContextualMenuClick? I believe it only selects words, not any object.

[Avi Drissman]

> Naming nit: shouldSelectWordOnContextualMenuClick? I believe it only selects words, not any object.

While I'm not attached to that name, it was deliberate. The Mac will select-and-contextual-menu anything you right-click on: files, text, etc. It feels more a consistent philosophy, so that's how I named it.

Note that if that's the only push-back I get on the patch I'm happy :)

[Antonio Gomes]

Comment on attachment 68460 [details]
Style fix

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

> WebCore/page/EventHandler.cpp:2010
> +    if (m_frame->editor()->behavior().shouldSelectOnContextualMenuClick()
> +        && !m_frame->selection()->contains(viewportPos)

Although I know your intention with this fix and I agree, as it is now you are going to regress Gtk, since it sets the Mac editing behavior.

See from webkitwebsettings.cpp:
(...)
 670     /**
 671     * WebKitWebSettings:editing-behavior
 672     *
 673     * This setting controls various editing behaviors that differ
 674     * between platforms and that have been combined in two groups,
 675     * 'Mac' and 'Windows'. Some examples:
 676     * 
 677     *  1) Clicking below the last line of an editable area puts the
 678     * caret at the end of the last line on Mac, but in the middle of
 679     * the last line on Windows.
 680     *
 681     *  2) Pushing down the arrow key on the last line puts the caret
 682     *  at the end of the last line on Mac, but does nothing on
 683     *  Windows. A similar case exists on the top line.
 684     *
 685     * Since: 1.1.13
 686     */
 687     g_object_class_install_property(gobject_class,
 688                                     PROP_EDITING_BEHAVIOR,
 689                                     g_param_spec_enum("editing-behavior",
 690                                                       _("Editing behavior"),
 691                                                       _("The behavior mode to use in editing mode"),
 692                                                       WEBKIT_TYPE_EDITING_BEHAVIOR,
 693                                                       WEBKIT_EDITING_BEHAVIOR_MAC,
 694                                                       flags));

So we really need a 'linux' behavior here. Could you please wait me to fix bug 36627 (I have time to work on that now)?

[Antonio Gomes]

(In reply to comment #9)
> (From update of attachment 68460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68460&action=review
> 
> > WebCore/editing/EditingBehavior.h:61
> > +    // On Mac, when processing a contextual click, the object being clicked upon should be selected.
> > +    bool shouldSelectOnContextualMenuClick() const { return m_type == EditingMacBehavior; }
> 
> Naming nit: shouldSelectWordOnContextualMenuClick? I believe it only selects words, not any object.

Hum, naming has to be well thought here, yes.

When you right-click a link, you get the whole link text selection. If it is a text node, only the clicked work gets selected.

[Avi Drissman]

(In reply to comment #11)
> So we really need a 'linux' behavior here. Could you please wait me to fix bug 36627 (I have time to work on that now)?

I don't mind waiting for you. If your plan is to hack on EditingBehavior to make it Linux-aware then that sounds great. When you're done, let me know how you want the new function (whatever it ends up named) to behave in your case.

[Avi Drissman]

(In reply to comment #11)
> Could you please wait me to fix bug 36627 (I have time to work on that now)?

Do you have an ETA on that bug? How's your progress?

[Antonio Gomes]

After thinking a bit more about this bug, and also after re-reading the discussion in bug 22261 , specially from https://bugs.webkit.org/show_bug.cgi?id=22261#c22 on, I am not sure anymore if we should make this an editing behavior. Well, I am ok with that, but would like to hear from Darin, first.

Darin, what do you think?

[Darin Adler]

There’s always a need to balance two conflicting goals with these:

    1) Have the web browser match the platform user interface conventions. Even more importantly, have software using web technology that is not a web browser match the platform standards.

    2) Have the web browser match the other web browsers on other platforms so websites work.

Historically, Windows got to have its cake and eat it too, since the web standards often exactly matched the conventions of the Windows platform. Things are trickier on other platforms such as Mac.

When it comes to what’s selected when you right-click, it’s critical when editing that Safari and Mail match the conventions of other Mac applications. A Mail user is not “using the web” so they have no reason to expect any particular behavior.

I don’t know how critically important this is on other platforms, nor specifically how this could affect website compatibility, so it’s hard for me to come up with a recommendation.

[Darin Adler]

(In reply to comment #16)
> A Mail user is not “using the web” so they have no reason to expect any particular behavior.

I meant, no reason to expect behavior different from other Mac applications that matches web browsers on Windows.

[Antonio Gomes]

Basically Mac is the only platform that seems to want it, for consistency within the platform, as you (Darin) said.

Chromium on Linux and Gtk (linux-only) have ifdef'ed out this behavior. Qt has a bug about not selecting the word under the right click. So I think we are ok with making this behavior Mac specific.

My only concern would be that we would be changing the behavior of Safari on Windows, for example, although it would be getting consistent with other applications on Windows.

If you have not objections, we will move this forward.

[Avi Drissman]

(In reply to comment #18)
> My only concern would be that we would be changing the behavior of Safari on Windows, for example, although it would be getting consistent with other applications on Windows.

Safari Windows is EditingMacBehavior?

[Avi Drissman]

(In reply to comment #18)
> My only concern would be that we would be changing the behavior of Safari on Windows, for example, although it would be getting consistent with other applications on Windows.

Sorry, I now understand. Yes, before this change WinSafari got the ifdef block and now it wouldn't. That would make it match the platform better. Is that OK?

[Antonio Gomes]

(In reply to comment #20)
> (In reply to comment #18)
> > My only concern would be that we would be changing the behavior of Safari on Windows, for example, although it would be getting consistent with other applications on Windows.
> 
> Sorry, I now understand. Yes, before this change WinSafari got the ifdef block and now it wouldn't. That would make it match the platform better. Is that OK?

That is my question :)

[Tony Chang]

Anecdotally, I will point out that in the early days of Windows Chrome, the selection on right click resulted in lots of complaints from users.  There are  Windows users who use the context menu back/forward entries for navigation.

I would suspect Safari Win will want to follow the platform convention on this.

[Ryosuke Niwa]

(In reply to comment #22)
> Anecdotally, I will point out that in the early days of Windows Chrome, the selection on right click resulted in lots of complaints from users.  There are  Windows users who use the context menu back/forward entries for navigation.
> 
> I would suspect Safari Win will want to follow the platform convention on this.

Can we add tests to ensure we're doing the right thing in each of three editing behaviors?  Windows, Mac, & Linux.  The latest patch doesn't have any tests and I don't want behavioral change like this landed without any tests.

[Darin Adler]

(In reply to comment #22)
> There are Windows users who use the context menu back/forward entries for navigation.

There are many Mac users who use the context menu back/forward entries for navigation. I’m not sure how this is relevant.

[Antonio Gomes]

(In reply to comment #24)
> (In reply to comment #22)
> > There are Windows users who use the context menu back/forward entries for navigation.
> 
> There are many Mac users who use the context menu back/forward entries for navigation. I’m not sure how this is relevant.

Darin, his point is that on Safari if you right click a text in the page (not a link), it will select the word and show up context menu with selection actions (copy, paste, ...). On Chromium/Mac it will not select the word and show up a context menu with navigation actions (back, forward, reload...).

[Ojan Vafai]

I don't think this is nearly as likely to have an impact on web compatibility as bug 22261. Also, as Tony pointed out, many users of Windows Chrome complained about this before we stopping doing it on Windows.*

I think we should do the platform-specific behavior here.
1. There are a non-trivial number of people that expect each platform's behavior.
2. This is not likely to have an impact on web compatibility or significantly complicate the code.
3. GTK, Chrome Windows and Chrome Linux already don't select the word on right-click.

So, the questions are whether QT/Safari-Win are OK with not selecting the word on right-click. Can people from QT and Safari-Win chime in if that seems OK?

As far as implementation, we need to have an EditingLinuxBehavior before we can fix this since GTK currently sets EditingMacBehavior.

* This diff doesn't show it, but elsewhere, Chrome does the correct Mac behavior of selecting the word on Chrome Mac.

[Antonio Gomes]

> So, the questions are whether QT/Safari-Win are OK with not selecting the word on right-click. Can people from QT and Safari-Win chime in if that seems OK?

Qt wants that as well.
 
> As far as implementation, we need to have an EditingLinuxBehavior before we can fix this since GTK currently sets EditingMacBehavior.

I started making all tests that involve touching platform specific behavior to run LayoutTestController::setEditingBehavior, so we can add the LinuxEditing behavior w/out any impact on existing tests. 

Here, we are just missing a OK about changing the safari behavior on Windows.

[Ojan Vafai]

Given that this is going to be platform-specific for all the other ports, it makes the most sense for it to be platform-specific for Safari as well. This doesn't seem controversial to me. Unless I hear objections from Safari folk, I'm comfortable approving this once the code is ready.

[Antonio Gomes]

(In reply to comment #28)
> Given that this is going to be platform-specific for all the other ports, it makes the most sense for it to be platform-specific for Safari as well. This doesn't seem controversial to me. Unless I hear objections from Safari folk, I'm comfortable approving this once the code is ready.

Agreed. There is enough Apple people cc'ed so they could let us know asap, and it moves on.

[Adam Roben (:aroben)]

(In reply to comment #26)
> So, the questions are whether QT/Safari-Win are OK with not selecting the word on right-click. Can people from QT and Safari-Win chime in if that seems OK?

I think this is probably fine for Safari on Windows.

[Antonio Gomes]

Avi, bug 36627 is fixed. Please provide a test case and the rest of the last patch can be essentially the same.

[Avi Drissman]

Created attachment 73666 [details]
New version, with layout test

This passes on my Mac; let's see what all the bots think.

[Hajime Morrita]

Created attachment 73693 [details]
Patch

[Hajime Morrita]

Comment on attachment 73666 [details]
New version, with layout test

I'm sorry that I accidentally cancelled the review. 
now restoring.....

[Hajime Morrita]

Comment on attachment 73693 [details]
Patch

This one was wrong. See avi's.

[Antonio Gomes]

Comment on attachment 73666 [details]
New version, with layout test

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

> WebCore/editing/EditingBehavior.h:61
> +    // On Mac/Unix, when processing a contextual click, the object being clicked upon should be selected.
> +    bool shouldSelectOnContextualMenuClick() const { return m_type == EditingMacBehavior || m_type == EditingUnixBehavior; }

Sorry Avi, maybe I misexplained: it is a Mac-specific behavior.

> LayoutTests/editing/selection/context-menu-text-selection.html:34
> +    var onWinPlatform = navigator.userAgent.search(/\bWindows\b/) != -1;
> +    if (onWinPlatform) {
> +        if (window.getSelection().type != "None")

This is not the right way to test this. See LayoutTestController::setEditingBehavior. grep for it. It basically makes it possible to test all editing behavior and having test expectations shared cross all platforms..

[Avi Drissman]

(In reply to comment #36)
> Sorry Avi, maybe I misexplained: it is a Mac-specific behavior.

I'm very confused now. You said that my original patch would regress GTK behavior, and you gave me a code snippet that seemed to set Mac behavior. So if my new patch that gives this behavior to Mac/Unix is wrong, what would you recommend?

> This is not the right way to test this. See LayoutTestController::setEditingBehavior. grep for it. It basically makes it possible to test all editing behavior and having test expectations shared cross all platforms..

I'll rewrite it. BTW, setEditingBehavior seems to take 'mac' and 'win' for parameters. What's the parameter for Unix? Whatever it is, I see that in our repo (webkit/tools/test_shell/layout_test_controller.cc) we don't have it.

[Antonio Gomes]

(In reply to comment #37)
> (In reply to comment #36)
> > Sorry Avi, maybe I misexplained: it is a Mac-specific behavior.
> 
> I'm very confused now. You said that my original patch would regress GTK behavior, and you gave me a code snippet that seemed to set Mac behavior. So if my new patch that gives this behavior to Mac/Unix is wrong, what would you recommend?
> 
You would regress GTK at that time. Now Gtk uses Linux behavior,not Mac. So only Mac wants this.

> > This is not the right way to test this. See LayoutTestController::setEditingBehavior. grep for it. It basically makes it possible to test all editing behavior and having test expectations shared cross all platforms..
> 
> I'll rewrite it. BTW, setEditingBehavior seems to take 'mac' and 'win' for parameters. What's the parameter for Unix? Whatever it is, I see that in our repo (webkit/tools/test_shell/layout_test_controller.cc) we don't have it.

It should be 'unix', but other tests have not being using it. Yours would be the first :)

[Avi Drissman]

Created attachment 74662 [details]
New version, uses setEditingBehavior

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74662 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html
animations/suspend-resume-animation-events.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74662 [details]
New version, uses setEditingBehavior

Rejecting patch 74662 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
mal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/RenderObject.o /Projects/CommitQueue/WebCore/rendering/RenderObject.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/RenderScrollbar.o /Projects/CommitQueue/WebCore/rendering/RenderScrollbar.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(47 failures)


Full output: http://queues.webkit.org/results/6357017

[Avi Drissman]

(In reply to comment #41)
> (From update of attachment 74662 [details])
> Rejecting patch 74662 from commit-queue.
> 
> Full output: http://queues.webkit.org/results/6357017

The output makes it unclear what went wrong. But with lines like:

i686-apple-darwin10-gcc-4.2.1: vfork: Operation timed out
/Developer/usr/bin/../libexec/gcc/i686-apple-darwin10/4.2.1/as: can't fork a new process to execute: /Developer/usr/bin/../libexec/gcc/darwin/x86_64/as (Resource temporarily unavailable)
/Projects/CommitQueue/WebCore/bindings/js/specialization/JSBindingState.cpp:63: fatal error: error writing to -: Broken pipe
compilation terminated.

It feels more like a sick bot than my code. Can someone advise?

[WebKit Commit Bot]

Comment on attachment 74662 [details]
New version, uses setEditingBehavior

Clearing flags on attachment: 74662

Committed r72618: <http://trac.webkit.org/changeset/72618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72618 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/context-menu-text-selection.html

[Antonio Gomes]

(In reply to comment #45)
> http://trac.webkit.org/changeset/72618 might have broken Qt Linux Release
> The following tests are not passing:
> editing/selection/context-menu-text-selection.html

CONSOLE MESSAGE: line 25: TypeError: 'undefined' is not an object (evaluating 'items.length')
This test checks that triggering the context menu selects/doesn't select as platform-appropriate.

Lorem ipsum
RUNNING
RUNNING
RUNNING

[Avi Drissman]

I asked Julie Parent to revert this change.

Can we re-open this bug? (Apparently I don't have perms.)

I have no idea why this is failing and am in a situation where I have no Linux boxes around. I'll look at it next week.

[Antonio Gomes]

Rolled out

[Julie Parent]

Rolled out in r72622.  New test failed for chromium-win and chromium-linux as well.

[Alexey Proskuryakov]

> (Apparently I don't have perms.)

Off topic for this bug, but I've now enabled canconfirm/editbugs for your account.

[Avi Drissman]

Created attachment 74764 [details]
With layout test fixed

Ouch. I'd copied the eventSender.contextClick() code from a different test and it turns out that only the new DRT actually returns something from eventSender.contextClick(). My mistake.

[WebKit Commit Bot]

Comment on attachment 74764 [details]
With layout test fixed

Clearing flags on attachment: 74764

Committed r72678: <http://trac.webkit.org/changeset/72678>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

This patch broke editing/selection/5109817.html and editing/selection/5354455-1.html on Windows. The former tests whether contextClick() triggers a selection, so seems to be superseded by context-menu-text-selection.html. The latter "tests whether right clicking on a paragraph break in editable content selects it". I'm going to check in new Windows results for these tests, but I think you should consider how to improve the situation.

[Avi Drissman]

(In reply to comment #54)

Apologies.

> This patch broke editing/selection/5109817.html and editing/selection/5354455-1.html on Windows. The former tests whether contextClick() triggers a selection, so seems to be superseded by context-menu-text-selection.html.

Agreed; the new test supersedes this one.

> The latter "tests whether right clicking on a paragraph break in editable content selects it".

New results make sense though if they're needed, what was the result earlier?

> I'm going to check in new Windows results for these tests, but I think you should consider how to improve the situation.

Rebaselining the second seems right to me; does deleting the first one sound good to you?

Avi

[Antonio Gomes]

(In reply to comment #54)
> This patch broke editing/selection/5109817.html and editing/selection/5354455-1.html on Windows. The former tests whether contextClick() triggers a selection, so seems to be superseded by context-menu-text-selection.html. The latter "tests whether right clicking on a paragraph break in editable content selects it". I'm going to check in new Windows results for these tests, but I think you should consider how to improve the situation.

Adam, at the very least then we could;

1) add LayoutTestController.setEditingBehavior('mac') and leave every thing as they were on these tests;

2) Or even better, if we think these tests have platform editing specific behavior, and we should fix the tests to test all possible code paths.

What do you think?

[Adam Roben (:aroben)]

(In reply to comment #55)
> (In reply to comment #54)
> Rebaselining the second seems right to me; does deleting the first one sound good to you?

Yes.

(In reply to comment #56)
> (In reply to comment #54)
> Adam, at the very least then we could;
> 
> 1) add LayoutTestController.setEditingBehavior('mac') and leave every thing as they were on these tests;
> 
> 2) Or even better, if we think these tests have platform editing specific behavior, and we should fix the tests to test all possible code paths.

I think doing (2) for 5354455-1.html would be good. I don't think we need to do it for the other test; we should just delete it.

[Avi Drissman]

(In reply to comment #57)
> (In reply to comment #55)
> > Rebaselining the second seems right to me; does deleting the first one sound good to you?
> 
> Yes.
> 
> (In reply to comment #56)
> I think doing (2) for 5354455-1.html would be good. I don't think we need to do it for the other test; we should just delete it.

OK then. Will do.

[Avi Drissman]

Created attachment 75156 [details]
Removal of superseded test, updating of other

[Darin Adler]

Comment on attachment 75156 [details]
Removal of superseded test, updating of other

The point of the test for 5109817 was specifically to check the behavior in non-editable text. Is there a test remaining that covers non-editable text? If not, then this test should be replaced or updated rather than removed.

I’ll be happy to change to review+ if you assure me there is a test that covers non-editable text.

[Avi Drissman]

(In reply to comment #60)
> The point of the test for 5109817 was specifically to check the behavior in non-editable text. Is there a test remaining that covers non-editable text? If not, then this test should be replaced or updated rather than removed.

I was unaware of 5109817 when I wrote the patch so I wrote context-menu-text-selection. It covers non-editable text in an editing-behavior-aware way and entirely supersedes 5109817. If there is something that 5109817 covered that context-menu-text-selection does not, please let me know.

[Avi Drissman]

Created attachment 75167 [details]
Updated with clarification in the description; missed a file for deletion

[Darin Adler]

(In reply to comment #61)
> I wrote context-menu-text-selection. It covers non-editable text

OK, great!

[WebKit Commit Bot]

Comment on attachment 75167 [details]
Updated with clarification in the description; missed a file for deletion

Clearing flags on attachment: 75167

Committed r73009: <http://trac.webkit.org/changeset/73009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.