Bug 164519 - Word transformations apply to a single word instead of entire selection if selection is made by triple-clicking
Summary: Word transformations apply to a single word instead of entire selection if se...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: data:text/html,<div%20contenteditable...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-08 10:13 PST by mitz
Modified: 2018-07-12 20:21 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2016-11-08 10:13:28 PST
<rdar://problem/27487147>

To reproduce:
Navigate to the URL, triple-click the sentence to select it, then control-click and choose Transformations > Make Upper Case

Result:
Only the word in which the control-click occurred is transformed and stays selected

Note:
The implementation of word transformations begins with performing the SelectWord command. The intent is to expand the selection to encompass whole words (that’s how the command behaves in NSTextView). When made with a triple-click, the selection’s base and extent are the same (the position of the click). It then expands to a word around that position. This happens in the VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity call from VisibleSelection::validate. It seems like we need a different code path for expanding to a desired granularity.
Comment 1 Darin Adler 2016-11-13 11:26:57 PST
The function VisibleSelection::expandUsingGranularity exists; is that relevant?
Comment 2 Darin Adler 2016-11-13 11:27:11 PST
And SelectionController::expandUsingGranularity.
Comment 3 mitz 2016-11-13 11:36:18 PST
(In reply to comment #1)
> The function VisibleSelection::expandUsingGranularity exists; is that
> relevant?

Yes, VisibleSelection::expandUsingGranularity is the one calling VisibleSelection::validate in this case. Here’s the call stack:

WebCore::VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity(WebCore::TextGranularity) at Source/WebCore/editing/VisibleSelection.cpp:272
WebCore::VisibleSelection::validate(WebCore::TextGranularity) at Source/WebCore/editing/VisibleSelection.cpp:422
WebCore::VisibleSelection::expandUsingGranularity(WebCore::TextGranularity) at Source/WebCore/editing/VisibleSelection.cpp:198
WebCore::expandSelectionToGranularity(WebCore::Frame&, WebCore::TextGranularity) at Source/WebCore/editing/EditorCommand.cpp:177
WebCore::executeSelectWord(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) at Source/WebCore/editing/EditorCommand.cpp:1015
WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const at Source/WebCore/editing/EditorCommand.cpp:1777
::-[WebHTMLView executeCoreCommandBySelector:](SEL) at Source/WebKit/mac/WebView/WebHTMLView.mm:2954
::-[WebHTMLView selectWord:](id) at Source/WebKit/mac/WebView/WebHTMLView.mm:3056
::-[WebHTMLView _changeWordCaseWithSelector:](SEL) at Source/WebKit/mac/WebView/WebHTMLView.mm:5824
::-[WebHTMLView uppercaseWord:](id) at Source/WebKit/mac/WebView/WebHTMLView.mm:5835

(In reply to comment #2)
> And SelectionController::expandUsingGranularity.

I don’t see that one in TOT.
Comment 4 Darin Adler 2016-11-13 11:45:25 PST
So I guess you are saying that this is a bug in WebCore::expandSelectionToGranularity/VisibleSelection::expandUsingGranularity.
Comment 5 mitz 2016-11-13 11:51:12 PST
(In reply to comment #4)
> So I guess you are saying that this is a bug in
> WebCore::expandSelectionToGranularity/VisibleSelection::
> expandUsingGranularity.

I am saying that those functions’ behavior doesn’t meet the requirements of word transformations. I don’t know how changing their behavior so that they never contract the selection will affect other uses. I also don’t know exactly how to do that. I tried, naively, to make VisibleSelection::expandUsingGranularity set the base and the extent to the start and end before calling validate, and then restore them, but that wasn’t quite right: it produced word selections that included the trailing space after the word.
Comment 6 Darin Adler 2016-11-13 16:53:10 PST
I think the trick is to make test cases first that express the behavior we want. I don’t think it will be terribly difficult to get the implementation right.

Certainly the name "expand selection to granularity" implies the behavior of never contracting selections, so if we need different behavior for some purposes, those should be in functions with different names!
Comment 7 Darin Adler 2016-11-13 16:56:32 PST
All those special cases in the WordGranularity section of setStartAndEndFromBaseAndExtentRespectingGranularity are probably the source of the difficulty with the trailing space after the word.

You mentioned setting the base and extent to the start and end before calling validate. Was the code you tried respecting m_baseIsFirst?
Comment 8 mitz 2016-11-13 16:59:49 PST
(In reply to comment #7)
> All those special cases in the WordGranularity section of
> setStartAndEndFromBaseAndExtentRespectingGranularity are probably the source
> of the difficulty with the trailing space after the word.
> 
> You mentioned setting the base and extent to the start and end before
> calling validate. Was the code you tried respecting m_baseIsFirst?

Yes, it was naïve but not that naïve. I will take a deeper look and try to understand what’s going on.
Comment 9 Darin Adler 2016-11-13 17:03:26 PST
Perhaps we should change things so the granularity is recorded alongside m_start and m_end. Then a call to expandUsingGranularity targeting any smaller granularity would automatically be a no-op.

This would not work for line vs. sentence granularity, but should work fine for character, word, sentence/line, paragraph, document.