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
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.
(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
Created attachment 68458 [details] Switch based on pref, not compile flag
Causes Chromium issues: http://code.google.com/p/chromium/issues/detail?id=53572 http://code.google.com/p/chromium/issues/detail?id=56371
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.
Created attachment 68460 [details] Style fix
This seems fine to me, but can someone working on QT verify that this is something the behavior they want? Antonio?
(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.
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.
> 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 :)
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)?
(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.
(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.
(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?
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?
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.
(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.
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.
(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?
(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?
(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 :)
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.
(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.
(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.
(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...).
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.
> 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.
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.
(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.
(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.
Avi, bug 36627 is fixed. Please provide a test case and the rest of the last patch can be essentially the same.
Created attachment 73666 [details] New version, with layout test This passes on my Mac; let's see what all the bots think.
Created attachment 73693 [details] Patch
Comment on attachment 73666 [details] New version, with layout test I'm sorry that I accidentally cancelled the review. now restoring.....
Comment on attachment 73693 [details] Patch This one was wrong. See avi's.
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..
(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.
(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 :)
Created attachment 74662 [details] New version, uses setEditingBehavior
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 cmarrin@apple.com and dumi@chromium.org. The commit-queue is continuing to process your patch.
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
(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?
Comment on attachment 74662 [details] New version, uses setEditingBehavior Clearing flags on attachment: 74662 Committed r72618: <http://trac.webkit.org/changeset/72618>
All reviewed patches have been landed. Closing bug.
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
(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
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.
Rolled out
Rolled out in r72622. New test failed for chromium-win and chromium-linux as well.
> (Apparently I don't have perms.) Off topic for this bug, but I've now enabled canconfirm/editbugs for your account.
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.
Comment on attachment 74764 [details] With layout test fixed Clearing flags on attachment: 74764 Committed r72678: <http://trac.webkit.org/changeset/72678>
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.
(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
(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?
(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.
(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.
Created attachment 75156 [details] Removal of superseded test, updating of other
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.
(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.
Created attachment 75167 [details] Updated with clarification in the description; missed a file for deletion
(In reply to comment #61) > I wrote context-menu-text-selection. It covers non-editable text OK, great!
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>