I think more about this approach. I think we should move handle visibility handling in content/renderer instead of in blink, because
1. It causes Web incompatibility. When handle visibility changed w/o change positions, it fires "selectionchange" event.
2. Layering violation: Since "Selection Handle" is UI gadget, it should not be in Blink core/.
3. Introduce code complexity and makes Blink to be hard to maintain [1][2]
4. "Selection Handler" makes Blink contributors harder to contribute to Blink. Not all contributors familiar with "Selection Handle" which is Android and ChromeOS feature.
[1] http://crrev.com/2370663002: Remove logic to reset input method more than needed
[2] http://crrev.com/2642913003: Select All should show handles if they were already present
Selection handles have become a fundamental selection feature on modern devices. Windows touch tablets also have them. So I believe it's appropriate for them to be in FrameSelection and for core Blink contributors to be forced to think about them. I could just as well complain that Blink core is full of mouse-specific logic like the distinction between start/end and base/extent which doesn't make sense for touch selections.
The refactoring to put this logic in Blink core eliminated significant complexity on the browser side. https://codereview.chromium.org/2201853002 is "491 insertions(+), 914 deletions(-)". We refactored it into Blink specifically to eliminate complexity because FrameSelection is in the natural place to make store state about visibility and is close to the editing logic that is relevant to the UX decisions.
We've thought about the remaining special cases for touch showing, and I think [1] and [2] cover the ones we know about and I don't believe there should be an explosion of further complex patches from here. There might be a couple more small ones, but the selection handle logic is almost complete and I expect we'll soon stop touching it in Blink.
TL;DR: Make handle visibility as first class citizen ==
Represent handle visibility in SelectionTemplate[1]
Since "Handle Visibility" is state of selection and we want to manage selection state in one place rather than spread into various place.
Rationale:
We've done same thing for following UI state into SelectionTemplate[1]:
TextAffinity m_affinity = TextAffinity::Downstream;
TextGranularity m_granularity = CharacterGranularity;
bool m_hasTrailingWhitespace = false;
bool m_isDirectional = false;
Before putting them in SelecitonTemplate, they are spread around the code like current "handle visibility".
>https://codereview.chromium.org/2201853002 is "491 insertions(+), 914 deletions(-)"
This is the reason I agreed.
>Blink core is full of mouse-specific logic like the distinction between start/end and base/extent which doesn't make sense for touch selections.
Concept of start/end will be removed[2], they are used for execCommand
bese/anchor (will be renamed to anchor/focus) are comes from specification[3]. Yes, anchor/focus comes from mouse and keyboard but this concept is also use in touch UI.
[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/editing/SelectionTemplate.h
[2] http://crbug.com/682886: Make VisibleSelection to align with Selection specification
[3] https://www.w3.org/TR/selection-api/
At this time, SelectionTemplate is just using for passing selection state as parameter. Hence, please keep FrameSelection::m_handleVisibility.
Once [1] is commited, SelectionTemplate is fundamental state of FrameSelection
[1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized positions
Sounds good to me, thanks! Then amaralp@ can write another patch to move the visibility state to SelectionTemplate after http://crrev.com/2637013002 is landed. We'll leave open this bug to track that.
We don't need to wait [1] because it takes time to land. [1] is a big patch and is needed to be split into multiple patch.
The plan is
1. Add m_handleVisibility to SelectionTemplate and SelectionTemplate::Builder
2. [2] and [3] can be done in parallel with using SelectionTemplate::Build
In SelectionTemplate, default value of m_handleVisible is hidden since most of usage of set selection hides handle.
We can call FrameSelection::setSelection() with
* Hide/Default case:
Just pass SelectionInXXX::Builder()...Build()
* Preserve case:
SelectionInXXX::Builder()...setHandleVisible(frame().selection().isHandleVisible()).Build()
*Show Handle case:
SelectionInXXX::Builder()...setHandleVisible(true).Build()
[1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized positions
[2] http://crrev.com/2642913003: Select All should show handles if they were already present
[3] http://crrev.com/2370663002: Remove logic to reset input method more than needed
Comment 1 by amaralp@chromium.org
, Aug 10 2016