Bug 23351

Summary: Add preference to not select when right-clicked
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: WebCore Misc.Assignee: Avi Drissman <avi>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, ap, aroben, avi, commit-queue, darin, eric, jparent, morrita, ojan, rniwa, tonikitoo, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 36627, 49757, 49987    
Bug Blocks: 39291, 50116    
Attachments:
Description Flags
Switch based on pref, not compile flag
none
Style fix
tonikitoo: review-, tonikitoo: commit-queue-
New version, with layout test
tonikitoo: review-
Patch
none
New version, uses setEditingBehavior
none
With layout test fixed
none
Removal of superseded test, updating of other
darin: review-, darin: commit-queue-
Updated with clarification in the description; missed a file for deletion none

Description Dimitri Glazkov (Google) 2009-01-15 11:30:10 PST
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
Comment 1 Ojan Vafai 2010-04-27 11:52:44 PDT
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.
Comment 2 Antonio Gomes 2010-04-27 12:15:29 PDT
(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
Comment 3 Avi Drissman 2010-09-22 15:40:59 PDT
Created attachment 68458 [details]
Switch based on pref, not compile flag
Comment 5 WebKit Review Bot 2010-09-22 15:43:22 PDT
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.
Comment 6 Avi Drissman 2010-09-22 15:47:10 PDT
Created attachment 68460 [details]
Style fix
Comment 7 Tony Chang 2010-09-22 15:55:18 PDT
This seems fine to me, but can someone working on QT verify that this is something the behavior they want?  Antonio?
Comment 8 Antonio Gomes 2010-09-22 16:15:42 PDT
(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 9 Ojan Vafai 2010-09-22 18:33:32 PDT
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.
Comment 10 Avi Drissman 2010-09-22 18:49:32 PDT
> 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 11 Antonio Gomes 2010-09-22 18:59:43 PDT
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)?
Comment 12 Antonio Gomes 2010-09-22 19:07:02 PDT
(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.
Comment 13 Avi Drissman 2010-09-22 19:10:01 PDT
(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.
Comment 14 Avi Drissman 2010-10-05 14:55:15 PDT
(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?
Comment 15 Antonio Gomes 2010-10-13 05:56:18 PDT
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?
Comment 16 Darin Adler 2010-10-13 09:37:12 PDT
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.
Comment 17 Darin Adler 2010-10-13 09:38:09 PDT
(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.
Comment 18 Antonio Gomes 2010-10-13 09:48:26 PDT
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.
Comment 19 Avi Drissman 2010-10-13 09:56:26 PDT
(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?
Comment 20 Avi Drissman 2010-10-13 10:06:57 PDT
(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?
Comment 21 Antonio Gomes 2010-10-13 10:30:35 PDT
(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 :)
Comment 22 Tony Chang 2010-10-13 10:59:12 PDT
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.
Comment 23 Ryosuke Niwa 2010-10-13 11:02:10 PDT
(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.
Comment 24 Darin Adler 2010-10-13 11:19:10 PDT
(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.
Comment 25 Antonio Gomes 2010-10-13 11:23:13 PDT
(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...).
Comment 26 Ojan Vafai 2010-10-21 18:00:44 PDT
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.
Comment 27 Antonio Gomes 2010-10-21 20:28:55 PDT
> 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.
Comment 28 Ojan Vafai 2010-10-22 08:00:20 PDT
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.
Comment 29 Antonio Gomes 2010-10-22 08:03:22 PDT
(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.
Comment 30 Adam Roben (:aroben) 2010-10-22 09:27:37 PDT
(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.
Comment 31 Antonio Gomes 2010-10-30 06:42:52 PDT
Avi, bug 36627 is fixed. Please provide a test case and the rest of the last patch can be essentially the same.
Comment 32 Avi Drissman 2010-11-11 14:51:41 PST
Created attachment 73666 [details]
New version, with layout test

This passes on my Mac; let's see what all the bots think.
Comment 33 Hajime Morrita 2010-11-11 19:42:42 PST
Created attachment 73693 [details]
Patch
Comment 34 Hajime Morrita 2010-11-11 19:43:44 PST
Comment on attachment 73666 [details]
New version, with layout test

I'm sorry that I accidentally cancelled the review. 
now restoring.....
Comment 35 Hajime Morrita 2010-11-11 19:44:41 PST
Comment on attachment 73693 [details]
Patch

This one was wrong. See avi's.
Comment 36 Antonio Gomes 2010-11-11 20:42:49 PST
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..
Comment 37 Avi Drissman 2010-11-12 08:01:34 PST
(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.
Comment 38 Antonio Gomes 2010-11-12 08:05:29 PST
(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 :)
Comment 39 Avi Drissman 2010-11-23 07:48:57 PST
Created attachment 74662 [details]
New version, uses setEditingBehavior
Comment 40 WebKit Commit Bot 2010-11-23 09:31:38 PST
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 41 WebKit Commit Bot 2010-11-23 10:39:49 PST
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
Comment 42 Avi Drissman 2010-11-23 11:09:25 PST
(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 43 WebKit Commit Bot 2010-11-23 11:34:16 PST
Comment on attachment 74662 [details]
New version, uses setEditingBehavior

Clearing flags on attachment: 74662

Committed r72618: <http://trac.webkit.org/changeset/72618>
Comment 44 WebKit Commit Bot 2010-11-23 11:34:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 45 WebKit Review Bot 2010-11-23 12:25:30 PST
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
Comment 46 Antonio Gomes 2010-11-23 12:29:10 PST
(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
Comment 47 Avi Drissman 2010-11-23 12:37:10 PST
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.
Comment 48 Antonio Gomes 2010-11-23 12:40:01 PST
Rolled out
Comment 49 Julie Parent 2010-11-23 12:55:09 PST
Rolled out in r72622.  New test failed for chromium-win and chromium-linux as well.
Comment 50 Alexey Proskuryakov 2010-11-23 23:12:08 PST
> (Apparently I don't have perms.)

Off topic for this bug, but I've now enabled canconfirm/editbugs for your account.
Comment 51 Avi Drissman 2010-11-24 08:40:15 PST
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 52 WebKit Commit Bot 2010-11-24 09:20:44 PST
Comment on attachment 74764 [details]
With layout test fixed

Clearing flags on attachment: 74764

Committed r72678: <http://trac.webkit.org/changeset/72678>
Comment 53 WebKit Commit Bot 2010-11-24 09:20:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 54 Adam Roben (:aroben) 2010-11-29 11:14:18 PST
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.
Comment 55 Avi Drissman 2010-11-29 11:22:46 PST
(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
Comment 56 Antonio Gomes 2010-11-29 12:48:36 PST
(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?
Comment 57 Adam Roben (:aroben) 2010-11-29 12:59:36 PST
(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.
Comment 58 Avi Drissman 2010-11-29 14:48:17 PST
(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.
Comment 59 Avi Drissman 2010-11-30 09:24:51 PST
Created attachment 75156 [details]
Removal of superseded test, updating of other
Comment 60 Darin Adler 2010-11-30 10:30:15 PST
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.
Comment 61 Avi Drissman 2010-11-30 10:44:13 PST
(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.
Comment 62 Avi Drissman 2010-11-30 10:51:03 PST
Created attachment 75167 [details]
Updated with clarification in the description; missed a file for deletion
Comment 63 Darin Adler 2010-11-30 10:55:28 PST
(In reply to comment #61)
> I wrote context-menu-text-selection. It covers non-editable text

OK, great!
Comment 64 WebKit Commit Bot 2010-11-30 21:37:44 PST
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>
Comment 65 WebKit Commit Bot 2010-11-30 21:37:54 PST
All reviewed patches have been landed.  Closing bug.