Bug 15643 – Double-clicking word should select whitespace after the word

• Component: HTML Editing
• Status: RESOLVED FIXED

[Yu-Hong Wang]

Double-clicking a word should select the whitespace that comes after the word. This is the default text selection behavior on Windows. WebKit's current behavior is that of the Mac OS.

[David Kilzer (:ddkilzer)]

Confirmed with WordPad and the Safari 310A24 seed on Windows XP Pro SP2.

[Glenn Wilson]

Created attachment 23130 [details]
Possible fix to issue 15643

Attached is a possible fix, with a platform-specific layout test.
*Credit should go to Yu-Hong Wang, who developed this fix.*

This fix changes double-clicking selection to include trailing spaces after the selected word.

Comment on attachment 23130 [details]
Possible fix to issue 15643

+#include <unicode/ubrk.h>

ICU is not available on all platforms, and should not be used directly like  that. You can use a TextBreakIterator.

+#if PLATFORM(WIN_OS)
I do not think this is the appropriate test, as the new behavior is not necessarily desirable for Safari on windows.

Finally, I don't see how this change is specific to selections created with double-click. It seems like it would affect any other selection. Selection::validate() is not the right place to introduce this change.

[Adam Roben (:aroben)]

(In reply to comment #3)
> +#if PLATFORM(WIN_OS)
> I do not think this is the appropriate test, as the new behavior is not
> necessarily desirable for Safari on windows.

I'd argue that the new behavior is not desirable for any version of WebKit that uses smart copy/smart delete/smart replace (as represented by EditorClient::smartInsertDeleteEnabled).

[Glenn Wilson]

Is the best scenario to check for is the case when the platform is Windows and smart editing is enabled?  I'd assume that we probably don't want this behavior on non-windows platforms, regardless of if smart editing is enabled or not.  It also seems like this check would go into EventHandler rather than Selection.

Also, it looks like a more appropriate method might be to check that "result.event()->clickCount() == 2" (or maybe modulus 2 == 0?) in EventHandler::selectClosestWordFromMouseEvent. And if so, call a new method inside Selection that uses TextBreakIterator to include trailing whitespace.  Does this seem like the right approach?

(In reply to comment #4)
> (In reply to comment #3)
> > +#if PLATFORM(WIN_OS)
> > I do not think this is the appropriate test, as the new behavior is not
> > necessarily desirable for Safari on windows.
> 
> I'd argue that the new behavior is not desirable for any version of WebKit that
> uses smart copy/smart delete/smart replace (as represented by
> EditorClient::smartInsertDeleteEnabled).
> 

[Glenn Wilson]

Created attachment 23176 [details]
Improved possible fix to bug 15643

Ok, here is an improved possible fix.

First, I added a method in Selection that will add trailing whitespace.  I wanted to keep things simple with a while loop, rather than use a TextBreakIterator.

This method will only be called by EventHandler::selectClosestWordFromMouseEvent  if on a Windows system, and the event was a mouse double-click.

I wasn't exactly sure how to check for smart editing, but it would be easy to add it to EventHandler's checks if necessary.

[Glenn Wilson]

Created attachment 23177 [details]
Improved possible fix for bug 15643

Oops -- had some tab leftovers in there.  Fixed now.

[Sam Weinig]

Comment on attachment 23177 [details]
Improved possible fix for bug 15643

I think this *does* need to check for smart editing.  I also think the test should use the eventSender.leapForward method instead of the setTimeout and should dumpAsText.

[Glenn Wilson]

So good news and bad news:
Good news: I have a new fix that checks the frame's editor for smartCopyOrDelete before it selects extra whitespace.  Also, I modified the test to use eventSender.leapForwards instead of setTimeouts (using dumpAsText too).  Yay!

Bad news: There's no way to have the test pass because, as far as I can tell, smart editing is always enabled by default (in Windows, too): last line of WebView::initWithFrame.  So it's always initialized as true, and is never reset anywhere else.

I'm not familiar with what smart Insert & Delete are supposed to do, at all.  Is there some way for the user to turn it off?  If it's always set to true, what's the use case for it to be false?

[Alexey Proskuryakov]

Some documentation on smart copy/insert: <http://developer.apple.com/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGUserInput/chapter_12_section_5.html>, <http://developer.apple.com/documentation/Cocoa/Conceptual/TextEditing/Tasks/Subclassing.html>.

One use case for having it disabled is editing non-natural text - e.g., Visual Studio and Xcode do not employ any such tricks with selections. I don't know which Mac WebKit clients use this, if any.

To me, it looks like selecting the trailing space is an alternative (and more limiting) UI for this same feature. I agree that it's far from a given that we would want it in Safari on Windows.

+#if PLATFORM(WIN)
I think that WIN is even worse here than WIN_OS, because it really means "Apple Windows port", and shouldn't be enabled for Chrome. But anyway, I think that this should be controlled via a separate client call, similarly to how smart delete and insert itself is controlled.

> Bad news: There's no way to have the test pass because, as far as I can tell,
> smart editing is always enabled by default (in Windows, too): last line of

Yes, DumpRenderTree should be extended to provide scriptable control over this.

[Glenn Wilson]

Ah, I see what you're saying now. Selection of whitespace after a word is nothing more than a workaround for intelligent insert and delete, which is already handled behind the scenes in WebKit anyway.

I guess there are three possible states of the world here: 
1.  Smart editing is enabled: functions as it does now.
2.  Smart editing is disabled, select-whitespace-after-word is enabled: trailing whitespace is selected as a work-around for intelligent editing
3.  Smart editing is disabled, select-whitespace-after-word is disabled: the client expressly desires both to be disabled for non-natural language editing

The fourth state, having both enabled, doesn't make sense: smart editing should trump the whitespace workaround, since they're both meant to solve the same problem.  This infers two boolean values, the whitespace workaround as a second configuration value to smartInsertDeleteEnabled.

To this end, I'd like to propose another EditorClient state variable and accompanying getter/setters: selectTrailingWhitespaceEnabled (bool). 
* Like smartInsertDeleteEnabled, it would live in Editor 
* When it is true and smartInsertDeleteEnabled is false, double-clicking a word will also select trailing whitespace
* It would be false by default, and the client would have to explicitly set it to true
* Setting it to true would have the side effect of setting smartInsertDelete to false
* This selection behavior would be platform independent

Does this make sense?  

[Alexey Proskuryakov]

Yes, sounds good to me.

I think that this is unlikely to be exposed as a WebKit API on Mac OS X, but having the code in WebCore (and testing in DumpRenderTree!) will do more good than harm.

[Glenn Wilson]

Created attachment 24774 [details]
In-progress patch to bug 15643

In-progress patch.  Still some cleanup and feedback needed on this.

[Glenn Wilson]

Created attachment 24782 [details]
Improved possible patch to issue 15643

Ok, here's a revised patch that implements what we talked about earlier in this issue.  Not sure if this is 100% right, so feedback is greatly appreciated!

This patch adds one more state variable to control when the client wants to use trailing whitespace selection instead of smart insert & delete.  This patch also adds APIs to the testing infrastructure to allow tests to manually turn this functionality on and off (to be able to test it.)

Not sure if this is totally right, but I tried to follow the smartInsertDelete API format as much as possible.  What should be improved here?

Thanks!!

[Alexey Proskuryakov]

Comment on attachment 24782 [details]
Improved possible patch to issue 15643

+        Added support for clients that wish to disable smart insert/delete
+        and enable the "trailing whitespace selection" work-around.

I'm not sure why you call this a work-around, it sounds like a normal feature to me.

It may make sense to add comments somewhere in code explaining the relationship between the two. Currently, one needs to go all the way to WebView to find out that the features are mutually exclusive, and that's a long way.

We normally explain individual changes (per-file or per-function) in ChangeLogs, but maybe this patch is obvious enough that there is no need to. 

+        * ChangeLog:

This line shouldn't be present in ChangeLogs.

+bool Editor::selectTrailingWhitespaceEnabled()

This starts with a verb, but doesn't select anything. I suggest naming this isSelectTrailingWhitespaceEnabled().

+void Selection::includeTrailingWhitespace()

Maybe appendTrailingWhitespace would be slightly more clear?

+        * WebView/WebView.h:

WebView.h is an Mac OS X API header, and I'm pretty sure that we don't want to expose this as API. Please modify WebViewPrivate.h instead. 

It is somewhat less of an issue on Windows, as we don't have a supported API there, so I'm not sure whether I should insist on doing the same on Windows. However, I believe that adding methods to the middle of IDL will break nightly builds.

r- for WebView.h, and for breaking nightly builds. Looks good otherwise!

[Glenn Wilson]

Created attachment 24822 [details]
Possible patch to issue 15643

Here's a revised patch for this issue.

>I'm not sure why you call this a work-around, it sounds like a normal feature
>to me.

That's fair.  I was thinking of it in terms of a work-around for smart insert/delete, but it really is a feature.  I've changed the ChangeLog text to reflect this.

>It may make sense to add comments somewhere in code explaining the relationship
>between the two. Currently, one needs to go all the way to WebView to find out
>that the features are mutually exclusive, and that's a long way.

Yes, agreed.  I put a comment in Editor.h, which I thought was high enough in the stack to be accessible but low enough to be generally applicable to what the underlying API is doing.  Let me know if this comment should say more or less, or if there's a better place than Editor.h.  (Editor.cpp?)

> This line shouldn't be present in ChangeLogs.

Yep...gone.

> This starts with a verb, but doesn't select anything. I suggest naming this
> isSelectTrailingWhitespaceEnabled().

Agreed...this was kind of confusing.  I've changed this to say "isSelectTrailing..." everywhere.

> Maybe appendTrailingWhitespace would be slightly more clear?

I wanted to make this consistent with the other methods added here, so I changed this to "selectTrailingWhitespace".  I thought that this has a chance to be confusing, as it infers that it is selecting *just* the trailing whitespace.  I also considered "includeTrailingWhitespace"...let me know if you'd like to see a different name here.

> WebView.h is an Mac OS X API header, and I'm pretty sure that we don't want to
> expose this as API. Please modify WebViewPrivate.h instead. 

Ok, I moved this into what I thought was the right interface in WebViewPrivate.  I'm not sure if the interface is right (WebPendingPublic).  Should this be in the WebPrivate interface?

> However, I believe that adding methods to the middle of IDL will break nightly
> builds.

I wasn't sure what you meant here, so I moved the methods to the very end of their interface in the IDL.   Did you mean that these have to be added to something other than the IDL, or changing IDLs dooms the nightlies to fail for a while?

Thanks again for the review!!

[Alexey Proskuryakov]

(In reply to comment #16)
> Yes, agreed.  I put a comment in Editor.h, which I thought was high enough in
> the stack to be accessible but low enough to be generally applicable to what
> the underlying API is doing.  Let me know if this comment should say more or
> less, or if there's a better place than Editor.h.  (Editor.cpp?)

Placing it to Editor.h seems good to me. I think that just the first two lines of the comment may be enough, but you may try asking someone who doesn't have the knowledge about smart select in cache memory :)

If you decide to go with the longer comment (which is fine with me), I'd suggest putting it in Editor.cpp.

> > Maybe appendTrailingWhitespace would be slightly more clear?
> 
> I wanted to make this consistent with the other methods added here, so I
> changed this to "selectTrailingWhitespace".  I thought that this has a chance
> to be confusing

Yes, I think that this new name is confusing, either the original "include" or "append" would be better.

>  I'm not sure if the interface is right (WebPendingPublic).  Should this be in
> the WebPrivate interface?

Yes, this should be private, as no one expressed the desire to make it a public Mac OS X API eventually.

> I wasn't sure what you meant here, so I moved the methods to the very end of
> their interface in the IDL.   Did you mean that these have to be added to
> something other than the IDL, or changing IDLs dooms the nightlies to fail for
> a while?

I'm not sure what the rules are. I'd try asking Adam Roben to advise on both Windows questions - whether it's OK to add this to WebView.h, and how to correctly modify the IDL.

[Adam Roben (:aroben)]

(In reply to comment #17)
> (In reply to comment #16)
> > I wasn't sure what you meant here, so I moved the methods to the very end of
> > their interface in the IDL.   Did you mean that these have to be added to
> > something other than the IDL, or changing IDLs dooms the nightlies to fail for
> > a while?
> 
> I'm not sure what the rules are. I'd try asking Adam Roben to advise on both
> Windows questions - whether it's OK to add this to WebView.h, and how to
> correctly modify the IDL.

Moving the new declarations to the end of the interface definition in the IDL file is the correct thing to do. This will ensure that the vtable that Safari expects this interface to have is unchanged (there will just be more information after what Safari considers to be the "end" of the vtable, which is harmless).

[Glenn Wilson]

Created attachment 24878 [details]
Possible fix for issue 15643

>Placing it to Editor.h seems good to me. I think that just the first two lines
>of the comment may be enough, but you may try asking someone who doesn't have
>the knowledge about smart select in cache memory :)

I took out all but the first two lines.  The bug number is in the changelogs, so I guess anyone really curious about the logic can check here :)

>Yes, I think that this new name is confusing, either the original "include" or
>"append" would be better.

I agree...changed to "appendTrailingWhitespace", which makes it very clear what the method is doing.

>Yes, this should be private, as no one expressed the desire to make it a public
>Mac OS X API eventually.

Ok, moved this out of WebPendingPublic to WebPrivate.

Since Adam mentioned that moving the new method signatures to the end of the interface in the IDL is the right approach, the IDL change shouldn't break the nightly builds.

Thanks for all of the great feedback!  Let me know if I missed anything...

[Alexey Proskuryakov]

Comment on attachment 24878 [details]
Possible fix for issue 15643

+        * ChangeLog:

Please remove this line from ChangeLogs (in fact, prepare-ChangeLog has been recently fixed to automatically omit it).

+    } else
+        document.write("PASS");

Why does this test say PASS if it didn't run at all? It should say that it can only run in automated mode.

r=me, but please fix the above issues when committing.

[Glenn Wilson]

Created attachment 24887 [details]
Possible patch for issue 15643

Ok, I went through and removed all the "Changelog:" lines in the patch.  Let me know if I missed any.

Also, I changed the layout test to explicitly say that the test must be run in automated mode, with access to LayoutTestController.  

Thanks for the review!

[Darin Fisher (:fishd, Google)]

Glenn, please post an updated patch.  This one no longer applies cleanly.

[Darin Fisher (:fishd, Google)]

Actually, the patch only conflicted on ChangeLog files.  I can resolve those myeslf.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38735

[Darin Fisher (:fishd, Google)]

Fixed gtk, qt, wx bustage:
http://trac.webkit.org/changeset/38738

[Darin Fisher (:fishd, Google)]

Fixed gtk DumpRenderTree link error:
http://trac.webkit.org/changeset/38739

[Tony Chang]

(In reply to comment #4)
> I'd argue that the new behavior is not desirable for any version of WebKit that
> uses smart copy/smart delete/smart replace (as represented by
> EditorClient::smartInsertDeleteEnabled).

As a contrary data point, smart insert/delete and select trailing whitespace are enabled on MS Word for Windows.  For that reason, we have both enabled in Chromium on Windows.