Bug 104186 – Draw selection and cursor handles for touch based text editing

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Varun Jain]

Draw selection and cursor handles for touch based text editing

[Varun Jain]

Created attachment 177861 [details]
Patch

[Varun Jain]

Hi James, This is the main patch for touch selection and editing as we talked about. This is not entirely ready for review (I am working on tests). But I wanted to put it up for early feedback. Please have a look

[James Robinson]

What objects here live on which threads?

[Varun Jain]

(In reply to comment #3)
> What objects here live on which threads?

TouchEditing object is created on the main thread with the creation of WebViewImpl. It is accessed on both threads, however, mostly on the main thread. Only access on the impl thread is the filterInputEvent call from WCIHi::handleInputEvent which checks if the event is on one of the handles (the TouchEditing::hitTest fucntion).

The TouchEditingController object (implemented by WebViewImpl) is entirely on main thread.

[James Robinson]

Comment on attachment 177861 [details]
Patch

This is definitely not threadsafe - the members on TouchEvents can be mutated on the main thread while the input handling logic is running on the compositor thread.

I think we should keep input handling in WebCompositorInputHandlerImpl and figure out how to get the information you need to that object in a thread-safe and consistent way.

[Varun Jain]

you are right that the members of TouchEditing can be mutated on the main thread while input handling logic runs on the impl thread. However, in this case I dont think it would be a problem. First, the main thread is writing values and impl thread is only reading them. So there are no concurrent writes. Second, even if in the rare case that impl thread reads an old value, it will just mean that we will hitTest wrongly and the handle will not move when its supposed to. If you'd like it better, I can make the hitTest() function thread safe so that hitTest() becomes an atomic operation.
That said, I am very much open to suggestions on other better ways of doing this. The main goal here is to intercept the scroll events before WCIHi gets them so that they can be used to move the handles instead of scrolling.

(In reply to comment #5)
> (From update of attachment 177861 [details])
> This is definitely not threadsafe - the members on TouchEvents can be mutated on the main thread while the input handling logic is running on the compositor thread.
> 
> I think we should keep input handling in WebCompositorInputHandlerImpl and figure out how to get the information you need to that object in a thread-safe and consistent way.

[Varun Jain]

Created attachment 178032 [details]
Patch

[Varun Jain]

Hi James, I added another patch with MutexLocking on hitTest() and updateGesmetry() functions. This makes the TouchEditing object thread safe. PTAL.

[James Robinson]

This is still wrong - you're hit testing the main thread's view of the world but the user is seeing the view of the world rendered by the compositor thread.  These can be out of sync in many ways - the main thread tree may be mutated, the compositor thread may have scrolled/zoomed/animated things around.  Your hit testing results will thus be completely inaccurate

I think you need to find someone with some experience in threaded input handling to give you some design feedback to make progress.  Perhaps James Maclean in your office can help?

[Varun Jain]

(In reply to comment #9)
> This is still wrong - you're hit testing the main thread's view of the world but the user is seeing the view of the world rendered by the compositor thread.  These can be out of sync in many ways - the main thread tree may be mutated, the compositor thread may have scrolled/zoomed/animated things around.  Your hit testing results will thus be completely inaccurate
> 
> I think you need to find someone with some experience in threaded input handling to give you some design feedback to make progress.  Perhaps James Maclean in your office can help?

Just talked to James (Maclean). He suggested some design changes that I will try to implement. Please ignore this patch (and the other one) for now. I will update  once I have implemented the new design.

[Varun Jain]

Created attachment 181426 [details]
Patch

[Varun Jain]

Created attachment 189226 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Varun Jain]

Hi James(R),
Apologies for the long absence on this due to my vacation. I will provide some context to refresh your memory: This patch adds TouchEditing class that draws handles for touch based selection and editing (such as selection handles or cursor handle to move the cursor in a textfield). The handles are drawn on their own layer (very much like LinkHighlight). These layers have setShouldScrollOnMainThread(true) because GestureScroll events on the handles need to be dealt with on the main thread because we would need to access/modify the current selection/cursor on the page. We have discussed this over chat before that this would not affect performance since only the events that happen on the handles go to the main thread. Other events on the page follow the usual impl thread route.
Please take a look.

[James Robinson]

(In reply to comment #14)
> Hi James(R),
> Apologies for the long absence on this due to my vacation. I will provide some context to refresh your memory: This patch adds TouchEditing class that draws handles for touch based selection and editing (such as selection handles or cursor handle to move the cursor in a textfield). The handles are drawn on their own layer (very much like LinkHighlight). These layers have setShouldScrollOnMainThread(true) because GestureScroll events on the handles need to be dealt with on the main thread because we would need to access/modify the current selection/cursor on the page. We have discussed this over chat before that this would not affect performance since only the events that happen on the handles go to the main thread. Other events on the page follow the usual impl thread route.
> Please take a look.

Thanks for the update and summary.  I'll take a close look at the patch at some point over the next few days, but one question first - why is this associated with a specific GraphicsLayer instead of using the PageOverlay mechanism already in place?

[Varun Jain]

(In reply to comment #15)
> (In reply to comment #14)
> > Hi James(R),
> > Apologies for the long absence on this due to my vacation. I will provide some context to refresh your memory: This patch adds TouchEditing class that draws handles for touch based selection and editing (such as selection handles or cursor handle to move the cursor in a textfield). The handles are drawn on their own layer (very much like LinkHighlight). These layers have setShouldScrollOnMainThread(true) because GestureScroll events on the handles need to be dealt with on the main thread because we would need to access/modify the current selection/cursor on the page. We have discussed this over chat before that this would not affect performance since only the events that happen on the handles go to the main thread. Other events on the page follow the usual impl thread route.
> > Please take a look.
> 
> Thanks for the update and summary.  I'll take a close look at the patch at some point over the next few days, but one question first - why is this associated with a specific GraphicsLayer instead of using the PageOverlay mechanism already in place?

This is done mostly to deal with animated text (for example, if one was to select the animated paragraph at  http://www.webkit.org/blog-files/pulse.html). We would want the selection handles to be in sync with the animation. So the layer has to hang off of the selected node's layer. Admittedly, I am not very familiar with the PageOverlay code, but AFAICT, it seems not possible (or at least very hard) to keep the pageoverlay in sync with a specific node.. is that correct? Regardless of that, IMHO, being associated with the specific GraphicsLayer makes more sense for this particular requirement.

[Varun Jain]

Created attachment 189625 [details]
Patch

[Varun Jain]

Hi James.. friendly ping :)

[James Robinson]

Comment on attachment 189625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189625&action=review

This patch isn't sufficiently tested given the rather large amount of implementation logic.  I've left a number of comments below to help you out.  A good number are stylistic things and issues with fitting in with the rest of the WebKit codebase.  For these sorts of issues you may find it more productive to work directly with someone who sits close to you who can guide you in the right direction.

At a higher level, I'd like to understand why you need to add new APIs for interaction between WebKit and the embedder for these selection handles.  We already have a fair amount of API by which WebKit and its embedder can keep track of and manipulate the current selection range.  It's not clear to me why we need more for these handles since they seem to simply sit around the selection range.  It would be helpful if you could describe the situations in which you expect these to be used.

> Source/WebKit/chromium/public/WebWidget.h:212
> +    // Called to inform the WebView that the selection/cursor on the webpage has changed.
> +    virtual void updateTouchEditing() { };

no trailing ;

Why does this feature need a new set of WebKit APIs?

> Source/WebKit/chromium/src/TouchEditing.cpp:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

it's 2013

> Source/WebKit/chromium/src/TouchEditing.cpp:42
> +#include "third_party/skia/include/utils/SkMatrix44.h"

in WebKit (unlike in chromium) this sort of include is written as just #include "SkMatrix44.h".  The dependency on the skia target adds it to the include path

> Source/WebKit/chromium/src/TouchEditing.cpp:57
> +namespace {

No need for an anonymous namespace if everything inside is static

> Source/WebKit/chromium/src/TouchEditing.cpp:71
> +    for (unsigned i = 0; i < 4; ++i) {
> +        IntPoint point;
> +        switch (i) {
> +        case 0: point = roundedIntPoint(targetSpaceQuad.p1()); break;
> +        case 1: point = roundedIntPoint(targetSpaceQuad.p2()); break;
> +        case 2: point = roundedIntPoint(targetSpaceQuad.p3()); break;
> +        case 3: point = roundedIntPoint(targetSpaceQuad.p4()); break;
> +        }

Really?  There's no better way to map a quad through a transform?  This is crazy ugly

> Source/WebKit/chromium/src/TouchEditing.cpp:87
> +static const int handlePadding = 20;
> +static const int handleRadius = 10;

Where did these values come from? How could they be changed?

> Source/WebKit/chromium/src/TouchEditing.cpp:140
> +        m_selectionHandle1.clear();
> +        m_selectionHandle2.clear();

"selectionHandle1" and "selectionHandle2" are terribly undescriptive names. Is there nothing better?  Does one handle represent the start and one the end or something like that?

> Source/WebKit/chromium/src/TouchEditing.cpp:195
> +    if (m_selectionHandle1.get()) {

no ".get()" for checking the nullity of a smart pointer in WebKit (or in chromium, for that matter).  just if (m_selectionHandle1)

> Source/WebKit/chromium/src/TouchEditing.cpp:197
> +            || m_selectionHandle2->handleEvent(eventType, location, controller);

does the existence of m_selelectionHandle1 imply that m_selectionHandle2 is always also not null?

> Source/WebKit/chromium/src/TouchEditing.cpp:298
> +    WebRect hitRect(m_rect.x - handleRadius - handlePadding,
> +        m_rect.y + m_rect.height,
> +        2 * (handleRadius + handlePadding),
> +        2 * (handleRadius + handlePadding));

there's a lot of nontrivial per-component math here and in the rest of the function.  I think you should familiarize yourself a bit with the WebCore geometry classes and use them to perform logical operations like "scale by this amount" or "translate by this offset".  This will make the code much easier to understand.

> Source/WebKit/chromium/src/TouchEditing.cpp:302
> +    return hitRect.x < location.x
> +        && hitRect.x + hitRect.width > location.x
> +        && hitRect.y < location.y
> +        && hitRect.y + hitRect.height > location.y;

For instance, bool WebCore::IntRect::contains(const IntPoint&) looks useful here (although you need to be careful about the edge cases)

> Source/WebKit/chromium/src/TouchEditing.cpp:333
> +    IntRect clipRect(IntPoint(webClipRect.x, webClipRect.y), IntSize(webClipRect.width, webClipRect.height));

WebRect defines operator WebCore::IntRect() const, why do you need these per-component operations?

> Source/WebKit/chromium/src/TouchEditing.cpp:336
> +    gc.setFillColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);
> +    gc.setStrokeColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);

these colors should be in a named constant with some comments indicating how they were chosen and what the procedure would be to update them

> Source/WebKit/chromium/src/TouchEditing.cpp:381
> +    m_clipLayer->setSublayerTransform(WebTransformationMatrix());
> +    if (!newGraphicsLayer->drawsContent()) {
> +        m_clipLayer->setSublayerTransform(WebTransformationMatrix(newGraphicsLayer->transform()));

I don't understand these sublayer transform modifications. What are they for?

> Source/WebKit/chromium/src/TouchEditing.h:54
> +namespace test {
> +class TouchEditingTest;
> +}

You shouldn't refer to test namespaces or classes in production code.

> Source/WebKit/chromium/src/TouchEditing.h:67
> +    virtual WebCore::FrameSelection* getCurrentSelection() = 0;
> +    virtual WebCore::GraphicsLayer* getRootGraphicsLayer() = 0;

getters in WebKit (functions that return a value) should not have a 'get' prefix. That prefix is used only when the function computes values into out params instead of returning them

> Source/WebKit/chromium/src/TouchEditing.h:94
> +    class TouchEditingHandle : public WebContentLayerClient {

It's quite rare to use inner classes in WebKit or to define multiple concrete classes in a single header.  I think you should define this class in its own header file with its own cpp

> Source/WebKit/chromium/src/TouchEditing.h:96
> +        TouchEditingHandle(TouchEditing*);

explicit for one-argument constructors

> Source/WebKit/chromium/src/TouchEditing.h:125
> +        TouchEditing* m_touchEditing;
> +        OwnPtr<WebContentLayer> m_contentLayer;
> +        OwnPtr<WebLayer> m_clipLayer;
> +        WebCore::Path m_path;
> +        WebCore::GraphicsLayerChromium* m_currentGraphicsLayer;
> +        bool m_hasCapture;
> +        WebRect m_rect;
> +        WebRect m_transformedRect;

why are these all public?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3582
> +bool WebViewImpl::handleInputEventForTouchEditing(const WebInputEvent& inputEvent)

This function has far too much code particular to touch handling to be in WebViewImpl, which is already an overburdened class.  Figure out a logical place for the details of handling this sort of input outside of WebViewImpl.cpp and move this code there

> Source/WebKit/chromium/src/WebViewImpl.h:630
> +    // Exposed for tests.
> +    TouchEditing* touchEditing() { return m_touchEditing.get(); }

Blank line before comment.  It's pretty unusual to expose classes like this just for tests.  I suspect if you extract more of the logic related to touch handling out of WebViewImpl you may not need to expose so many of WebViewImpl's internals to your tests.

> Source/WebKit/chromium/src/WebViewImpl.h:686
> +    bool touchEditingEnabled();
> +    bool pageHasSelectionOrCaret();
> +    bool handleInputEventForTouchEditing(const WebInputEvent&);
> +    void createTouchEditing();
> +    void clearTouchEditing();

5 new helper functions for this?  Ouch

> Source/WebKit/chromium/tests/TouchEditingTest.cpp:88
> +TEST_F(TouchEditingTest, verifyWebViewImplIntegration)

Adding 'verify' to a unit test name doesn't add anything.  All tests exists to verify something, that's why they are tests

> Source/WebKit/chromium/tests/TouchEditingTest.cpp:137
> +    // Now tap else where to hide all handles.

There seem to be far fewer test scenarios than cases in the code under test.  For instance, I don't see tests for any of the RenderLayer / transform / etc logic.  Make sure that your tests all of the logic they are intended to cover.

[Varun Jain]

Created attachment 190134 [details]
Patch

[Varun Jain]

Hi James, Thanks for the review. First, apologies for the styling mistakes. I suppose I should send webkit patches more often :| While I have correct all the ones you pointed out, if you still think necessary, I will get this reviewed for style from someone else to save you the time, while you can review for design and logic.
I have addressed/replied to all your comments inline below. Your two major concerns were:
1. lack of testing: Added more tests for selection of transformed and scrolled text
2. reason for new APIs: There are two new APIs I am adding:
- WebWidget::updateTouchEditing: reason explained inline below your comment.
- WebViewClient::didChangeTouchEditingHandleVisibility: The basic requirement here is that WebKit inform the embedder that selection or cursor handles have shown/hidden so that the embedder can take appropriate action (such as show a cut/copy/paste widget). Webkit does inform the embedder of selection change and also input type change. This is however insufficient because there are quite a few cases in which these triggers do not mean that selection handles are shown. For instance, we do not show handles for selection using mouse. We also do not show cursor handle when the input type changes, instead we show it when the user taps the second time on the input element. There are more instances. Due to this, I think this is a necessary API addition through which the embedder can definitively know when the handles are shown/hidden.

(In reply to comment #19)
> (From update of attachment 189625 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189625&action=review
> 
> This patch isn't sufficiently tested given the rather large amount of implementation logic.  I've left a number of comments below to help you out.  A good number are stylistic things and issues with fitting in with the rest of the WebKit codebase.  For these sorts of issues you may find it more productive to work directly with someone who sits close to you who can guide you in the right direction.
> 
> At a higher level, I'd like to understand why you need to add new APIs for interaction between WebKit and the embedder for these selection handles.  We already have a fair amount of API by which WebKit and its embedder can keep track of and manipulate the current selection range.  It's not clear to me why we need more for these handles since they seem to simply sit around the selection range.  It would be helpful if you could describe the situations in which you expect these to be used.
> 
> > Source/WebKit/chromium/public/WebWidget.h:212
> > +    // Called to inform the WebView that the selection/cursor on the webpage has changed.
> > +    virtual void updateTouchEditing() { };
> 
> no trailing ;

Done.

> 
> Why does this feature need a new set of WebKit APIs?
> 

The reason for this API is as follows: We want the touch handles to be up to date with the selection and the "cut/copy/paste" menu all the time. Syncing with the selection boundaries is easy because both the handles and the selection are drawn in webkit. The menu on the other hand is drawn by the embedder. The embedder draws the menu using its state of selection boundaries (done in RenderWidget::UpdateSelectionBounds). So I thought that the most definitive way to ensure that the menu and handles stay in sync is to let the embedder update the handles whenever it updates its local state.

> > Source/WebKit/chromium/src/TouchEditing.cpp:2
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> it's 2013

Done.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:42
> > +#include "third_party/skia/include/utils/SkMatrix44.h"
> 
> in WebKit (unlike in chromium) this sort of include is written as just #include "SkMatrix44.h".  The dependency on the skia target adds it to the include path
> 

Done.

> > Source/WebKit/chromium/src/TouchEditing.cpp:57
> > +namespace {
> 
> No need for an anonymous namespace if everything inside is static
> 

Done.

> > Source/WebKit/chromium/src/TouchEditing.cpp:71
> > +    for (unsigned i = 0; i < 4; ++i) {
> > +        IntPoint point;
> > +        switch (i) {
> > +        case 0: point = roundedIntPoint(targetSpaceQuad.p1()); break;
> > +        case 1: point = roundedIntPoint(targetSpaceQuad.p2()); break;
> > +        case 2: point = roundedIntPoint(targetSpaceQuad.p3()); break;
> > +        case 3: point = roundedIntPoint(targetSpaceQuad.p4()); break;
> > +        }
> 
> Really?  There's no better way to map a quad through a transform?  This is crazy ugly

Done.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:87
> > +static const int handlePadding = 20;
> > +static const int handleRadius = 10;
> 
> Where did these values come from? How could they be changed?
> 

These are determined experimentally (ie, I played around with it for a few mins and arrived at these). handleRadius is the radius of the circular selection handle. Currently I am drawing the selection handle inline using graphics commands but most likely, in the future, it will be drawn from a resource, in which case, this constant will go away.
handlePadding is the padding around the selection handle that specifies the area included in hit-testing (so that its easier to hit the handle with finger). Its value can be adjusted if people find it too hard or too easy to hit the selection handles in practice.

> > Source/WebKit/chromium/src/TouchEditing.cpp:140
> > +        m_selectionHandle1.clear();
> > +        m_selectionHandle2.clear();
> 
> "selectionHandle1" and "selectionHandle2" are terribly undescriptive names. Is there nothing better?  Does one handle represent the start and one the end or something like that?
> 

the names are oblivious of start and end on purpose because webcore returns the selection bounds as represented by the logical start and logical end of the selection. So for example, with the initial selection, we set selectionHandle1 to be the logical start and selectionHandle2 to be the logical end. Now if the user drags selectionHandle1, then it becomes the end and selectionHandle2 becomes the start.

> > Source/WebKit/chromium/src/TouchEditing.cpp:195
> > +    if (m_selectionHandle1.get()) {
> 
> no ".get()" for checking the nullity of a smart pointer in WebKit (or in chromium, for that matter).  just if (m_selectionHandle1)

Done.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:197
> > +            || m_selectionHandle2->handleEvent(eventType, location, controller);
> 
> does the existence of m_selelectionHandle1 imply that m_selectionHandle2 is always also not null?
> 

Yes. Fixed.

> > Source/WebKit/chromium/src/TouchEditing.cpp:298
> > +    WebRect hitRect(m_rect.x - handleRadius - handlePadding,
> > +        m_rect.y + m_rect.height,
> > +        2 * (handleRadius + handlePadding),
> > +        2 * (handleRadius + handlePadding));
> 
> there's a lot of nontrivial per-component math here and in the rest of the function.  I think you should familiarize yourself a bit with the WebCore geometry classes and use them to perform logical operations like "scale by this amount" or "translate by this offset".  This will make the code much easier to understand.

Done. Also added comments.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:302
> > +    return hitRect.x < location.x
> > +        && hitRect.x + hitRect.width > location.x
> > +        && hitRect.y < location.y
> > +        && hitRect.y + hitRect.height > location.y;
> 
> For instance, bool WebCore::IntRect::contains(const IntPoint&) looks useful here (although you need to be careful about the edge cases)

Done.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:333
> > +    IntRect clipRect(IntPoint(webClipRect.x, webClipRect.y), IntSize(webClipRect.width, webClipRect.height));
> 
> WebRect defines operator WebCore::IntRect() const, why do you need these per-component operations?

Removed.

> 
> > Source/WebKit/chromium/src/TouchEditing.cpp:336
> > +    gc.setFillColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);
> > +    gc.setStrokeColor(Color(0, 0, 0, 128), ColorSpaceDeviceRGB);
> 
> these colors should be in a named constant with some comments indicating how they were chosen and what the procedure would be to update them
> 

Done.

> > Source/WebKit/chromium/src/TouchEditing.cpp:381
> > +    m_clipLayer->setSublayerTransform(WebTransformationMatrix());
> > +    if (!newGraphicsLayer->drawsContent()) {
> > +        m_clipLayer->setSublayerTransform(WebTransformationMatrix(newGraphicsLayer->transform()));
> 
> I don't understand these sublayer transform modifications. What are they for?
> 

Leftover from copied code :(.. removed.

> > Source/WebKit/chromium/src/TouchEditing.h:54
> > +namespace test {
> > +class TouchEditingTest;
> > +}
> 
> You shouldn't refer to test namespaces or classes in production code.

I see this being done elsewhere in the code (ScrollAnimatorNone.h, Canvas2DLayerManager.h, TransparencyWin.h). This is only for testing so that unit tests have access to the private fields of TouchEditing. The other option would be to create public accessors for all those fields which sounds ugly.

> 
> > Source/WebKit/chromium/src/TouchEditing.h:67
> > +    virtual WebCore::FrameSelection* getCurrentSelection() = 0;
> > +    virtual WebCore::GraphicsLayer* getRootGraphicsLayer() = 0;
> 
> getters in WebKit (functions that return a value) should not have a 'get' prefix. That prefix is used only when the function computes values into out params instead of returning them
> 

Done.

> > Source/WebKit/chromium/src/TouchEditing.h:94
> > +    class TouchEditingHandle : public WebContentLayerClient {
> 
> It's quite rare to use inner classes in WebKit or to define multiple concrete classes in a single header.  I think you should define this class in its own header file with its own cpp
> 

Done.

> > Source/WebKit/chromium/src/TouchEditing.h:96
> > +        TouchEditingHandle(TouchEditing*);
> 
> explicit for one-argument constructors

Done.

> 
> > Source/WebKit/chromium/src/TouchEditing.h:125
> > +        TouchEditing* m_touchEditing;
> > +        OwnPtr<WebContentLayer> m_contentLayer;
> > +        OwnPtr<WebLayer> m_clipLayer;
> > +        WebCore::Path m_path;
> > +        WebCore::GraphicsLayerChromium* m_currentGraphicsLayer;
> > +        bool m_hasCapture;
> > +        WebRect m_rect;
> > +        WebRect m_transformedRect;
> 
> why are these all public?
> 

Done.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3582
> > +bool WebViewImpl::handleInputEventForTouchEditing(const WebInputEvent& inputEvent)
> 
> This function has far too much code particular to touch handling to be in WebViewImpl, which is already an overburdened class.  Figure out a logical place for the details of handling this sort of input outside of WebViewImpl.cpp and move this code there
> 

The handling can go either in TouchEditing class, in which case I will need to expose a few methods on WebViewImpl since the handling accesses those internals. Or it can go in EventHandler, which seems completely wrong to me because this is very touch editing specific :(
I will think about this more but if you have any suggestions I would greatly appreciate it.

> > Source/WebKit/chromium/src/WebViewImpl.h:630
> > +    // Exposed for tests.
> > +    TouchEditing* touchEditing() { return m_touchEditing.get(); }
> 
> Blank line before comment.  It's pretty unusual to expose classes like this just for tests.  I suspect if you extract more of the logic related to touch handling out of WebViewImpl you may not need to expose so many of WebViewImpl's internals to your tests.
> 

Added blank line. Regarding exposing for tests: There are quite a few other instances in this class itself (for example, all the *ForTesting() methods and also linkHighlight()). I will see however, if this can be avoided once I figure out what to do with moving the touch handling outside of WebViewImpl.

> > Source/WebKit/chromium/src/WebViewImpl.h:686
> > +    bool touchEditingEnabled();
> > +    bool pageHasSelectionOrCaret();
> > +    bool handleInputEventForTouchEditing(const WebInputEvent&);
> > +    void createTouchEditing();
> > +    void clearTouchEditing();
> 
> 5 new helper functions for this?  Ouch
> 

I am not sure what you mean. Is there any specific one of these that you think is not necessary or should be inlined?

> > Source/WebKit/chromium/tests/TouchEditingTest.cpp:88
> > +TEST_F(TouchEditingTest, verifyWebViewImplIntegration)
> 
> Adding 'verify' to a unit test name doesn't add anything.  All tests exists to verify something, that's why they are tests
> 

Done.

> > Source/WebKit/chromium/tests/TouchEditingTest.cpp:137
> > +    // Now tap else where to hide all handles.
> 
> There seem to be far fewer test scenarios than cases in the code under test.  For instance, I don't see tests for any of the RenderLayer / transform / etc logic.  Make sure that your tests all of the logic they are intended to cover.

Added more tests for transformed and scrolled text.

[James Robinson]

I still don't follow the API additions.  When would the presence of handles change what the embedder would do?  I would think things like the cut/copy/paste menu would depend purely on the selection state.

I think it would be useful to split this patch into two pieces, one that deals exclusively with rendering the handles and another that deals with any new embedder interactions.

[Varun Jain]

(In reply to comment #22)
> I still don't follow the API additions.  When would the presence of handles change what the embedder would do?  I would think things like the cut/copy/paste menu would depend purely on the selection state.
> 

I hope you are convinced by my explanation of the need for the WebViewClient::didChangeTouchEditingHandleVisibility API.
Let me further clarify the need for WebWidget::updateTouchEditing API. I do agree that presence of handles is totally dependent upon the selection state and should not depend on the embedder. However, in the current state, the only way to receive a notification from WebCore about a possible selection change is through the WebViewClient::didChangeSelection() method which is implemented by the embedder. Note that the selection change can occur even in the absence of any user events because the webpage can manipulate selection. These changes will not affect anything in WebViewImpl and in such case, AFAICT, the WebViewClient::didChangeSelection callback is the only way we can update the handles. For the embedder implementation of didChangeSelection to be able to update the handles, I need this API.

> I think it would be useful to split this patch into two pieces, one that deals exclusively with rendering the handles and another that deals with any new embedder interactions.

I will split the patch. Lets resolve the API issues in the meantime.

[Varun Jain]

Created attachment 190822 [details]
Patch

[Varun Jain]

Hi James.. API changes moved to https://bugs.webkit.org/show_bug.cgi?id=111120
PTAL.

[Varun Jain]

Created attachment 190863 [details]
Patch

[Varun Jain]

Created attachment 191272 [details]
Patch

[WebKit Review Bot]

Comment on attachment 191272 [details]
Patch

Attachment 191272 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16851444

[WebKit Review Bot]

Comment on attachment 191272 [details]
Patch

Attachment 191272 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16967058

[James Robinson]

Comment on attachment 191272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review

I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.

> Source/WebCore/page/Settings.in:200
> +# This setting enables touch based text selection and editing.
> +touchEditingEnabled initial=false

this appears to be a chromium-specific feature (at least, all of the implementation logic is in .../chromium/... files), so it shouldn't be in the cross-platform Settings file

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:66
> +    virtual void invalidate() = 0;
> +    virtual void clearCurrentGraphicsLayer() = 0;
> +    virtual void layers(Vector<WebKit::WebLayer*>&) = 0;

The behavior of these functions doesn't seem very clear from the signatures.  What does "invalidate" invalidate?  What does "layers" return?  They need clearer names and possibly some comments (although self-descriptive names are better)

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:209
> +    TouchEditingClient* m_touchEditing;

What's a "touch editing"?  That doesn't seem noun-like.

[Varun Jain]

(In reply to comment #30)
> (From update of attachment 191272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> 
> I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> 

we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)


> > Source/WebCore/page/Settings.in:200
> > +# This setting enables touch based text selection and editing.
> > +touchEditingEnabled initial=false
> 
> this appears to be a chromium-specific feature (at least, all of the implementation logic is in .../chromium/... files), so it shouldn't be in the cross-platform Settings file
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:66
> > +    virtual void invalidate() = 0;
> > +    virtual void clearCurrentGraphicsLayer() = 0;
> > +    virtual void layers(Vector<WebKit::WebLayer*>&) = 0;
> 
> The behavior of these functions doesn't seem very clear from the signatures.  What does "invalidate" invalidate?  What does "layers" return?  They need clearer names and possibly some comments (although self-descriptive names are better)
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:209
> > +    TouchEditingClient* m_touchEditing;
> 
> What's a "touch editing"?  That doesn't seem noun-like.

[James Robinson]

(In reply to comment #31)
> (In reply to comment #30)
> > (From update of attachment 191272 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > 
> > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > 
> 
> we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)

You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.

[Varun Jain]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > (From update of attachment 191272 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > > 
> > > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > > 
> > 
> > we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)
> 
> You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.

Hi James, there are a few problems with drawing on the content itself:
1. the handles will be clipped to the clip of the layer that the content is on
2. In order to update the handles, the whole layer will need to be re-drawn
3. In future, I do plan to fade the handles in/out

Also, I mentioned in my previous comment that the handles are drawn on a separate compositing layer. That was a mistake. I am simply creating a separate layer.

[James Robinson]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #30)
> > > > (From update of attachment 191272 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > > > 
> > > > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > > > 
> > > 
> > > we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)
> > 
> > You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.
> 
> Hi James, there are a few problems with drawing on the content itself:
> 1. the handles will be clipped to the clip of the layer that the content is on

Hmm, so you want a different clipping hierarchy for the handles?  That will need some additional work (beyond what you've done here) to figure out what the correct clip is and how to produce that clipping hierarchy.  This could be a lot of work depending on what it needs to do.  What is the exact clipping behavior you want?  Where are the tests for this behavior?

The link highlight system, which it appears you are mimicing, doesn't need to solve this problem since it has the same clipping requirements as the link itself.

> 2. In order to update the handles, the whole layer will need to be re-drawn

No, only the parts that have changed (i.e. under the handles).

> 3. In future, I do plan to fade the handles in/out

It'll be really helpful if you can fully describe the behavior you want up front so we can make sure we're trying to solve the same problem.

> 
> Also, I mentioned in my previous comment that the handles are drawn on a separate compositing layer. That was a mistake. I am simply creating a separate layer.

[Peter Beverloo (cr-android ews)]

Comment on attachment 191272 [details]
Patch

Attachment 191272 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16984011

[Varun Jain]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (In reply to comment #30)
> > > > > (From update of attachment 191272 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > > > > 
> > > > > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > > > > 
> > > > 
> > > > we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)
> > > 
> > > You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.
> > 
> > Hi James, there are a few problems with drawing on the content itself:
> > 1. the handles will be clipped to the clip of the layer that the content is on
> 
> Hmm, so you want a different clipping hierarchy for the handles?  That will need some additional work (beyond what you've done here) to figure out what the correct clip is and how to produce that clipping hierarchy.  This could be a lot of work depending on what it needs to do.  What is the exact clipping behavior you want?  Where are the tests for this behavior?
> 
> The link highlight system, which it appears you are mimicing, doesn't need to solve this problem since it has the same clipping requirements as the link itself.

Currently the handles will be clipped to the parent compositing layer's clip (if it has one). I think this works well because only layers such as scrollable divs, iframes etc. have a clip and we want the handles to respect that clip (if the selected text is not visible, we dont want the handles to be visible either). Changing the clip hierarchy seems to be very hard and I am not sure we should do that.

> 
> > 2. In order to update the handles, the whole layer will need to be re-drawn
> 
> No, only the parts that have changed (i.e. under the handles).

Yes. You are right.

> 
> > 3. In future, I do plan to fade the handles in/out
> 
> It'll be really helpful if you can fully describe the behavior you want up front so we can make sure we're trying to solve the same problem.
> 

Sorry for misguiding you. We do have the fade in/out requirement. However, I am curious to know what is so bad about drawing these as a separate layer instead of on the content. From the top of it, it seems more efficient to have a separate layer to avoid redrawing (even the small portion under the handle) the content layer. More so because these layers will be added only when there is selection on the page and the user is interacting using touch. Otherwise, the layers do not add to anything performance-wise. But maybe I am missing something important here.
In any case, I think the fade requirement makes a strong case for separating the layer. If you agree, I can go ahead with this approach and attend to the other comments you gave earlier.

> > 
> > Also, I mentioned in my previous comment that the handles are drawn on a separate compositing layer. That was a mistake. I am simply creating a separate layer.

[James Robinson]

(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > (In reply to comment #32)
> > > > (In reply to comment #31)
> > > > > (In reply to comment #30)
> > > > > > (From update of attachment 191272 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > > > > > 
> > > > > > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > > > > > 
> > > > > 
> > > > > we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)
> > > > 
> > > > You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.
> > > 
> > > Hi James, there are a few problems with drawing on the content itself:
> > > 1. the handles will be clipped to the clip of the layer that the content is on
> > 
> > Hmm, so you want a different clipping hierarchy for the handles?  That will need some additional work (beyond what you've done here) to figure out what the correct clip is and how to produce that clipping hierarchy.  This could be a lot of work depending on what it needs to do.  What is the exact clipping behavior you want?  Where are the tests for this behavior?
> > 
> > The link highlight system, which it appears you are mimicing, doesn't need to solve this problem since it has the same clipping requirements as the link itself.
> 
> Currently the handles will be clipped to the parent compositing layer's clip (if it has one). I think this works well because only layers such as scrollable divs, iframes etc. have a clip and we want the handles to respect that clip (if the selected text is not visible, we dont want the handles to be visible either). Changing the clip hierarchy seems to be very hard and I am not sure we should do that.

Do you have specific examples?

> > > 3. In future, I do plan to fade the handles in/out
> > 
> > It'll be really helpful if you can fully describe the behavior you want up front so we can make sure we're trying to solve the same problem.
> > 
> 
> Sorry for misguiding you. We do have the fade in/out requirement. However, I am curious to know what is so bad about drawing these as a separate layer instead of on the content. From the top of it, it seems more efficient to have a separate layer to avoid redrawing (even the small portion under the handle) the content layer. More so because these layers will be added only when there is selection on the page and the user is interacting using touch. Otherwise, the layers do not add to anything performance-wise. But maybe I am missing something important here.
> In any case, I think the fade requirement makes a strong case for separating the layer. If you agree, I can go ahead with this approach and attend to the other comments you gave earlier.

My concern is having separate manually managed layers is very complicated and will require a lot of careful work to get correct and maintain, as evidenced by all the math in this patch and the tests that will need to be written.  I'd like to make sure we know exactly what UX we are implementing and have all of the requirements and constraints in mind before committing on a specific path.

[James Robinson]

Comment on attachment 191272 [details]
Patch

What's the expected interaction when the user is inputting a mix of touch events and mouse/keyboard events?  Can the handles be manipulated by the mouse?

If the handles are up and the user manipulates the selection with the keyboard or mouse, do the handles move?  Do they go away?

If the handles are up and the selection is modified by the page, do the handles move?  Do they go away?

[Varun Jain]

(In reply to comment #38)
> (From update of attachment 191272 [details])
> What's the expected interaction when the user is inputting a mix of touch events and mouse/keyboard events?  Can the handles be manipulated by the mouse?
> 

The handles should not be manipulatable by mouse. This is taken care of in my patch in the WebViewImpl::handleInputEventForTouchEditing() method. Basically, if at any point if the handles are up and we receive an event that is not touch, we remove the handles.

> If the handles are up and the user manipulates the selection with the keyboard or mouse, do the handles move?  Do they go away?
> 

Yes.

> If the handles are up and the selection is modified by the page, do the handles move?  Do they go away?

The handles should update to the new selection.

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #34)
> > > (In reply to comment #33)
> > > > (In reply to comment #32)
> > > > > (In reply to comment #31)
> > > > > > (In reply to comment #30)
> > > > > > > (From update of attachment 191272 [details] [details] [details] [details] [details] [details] [details])
> > > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=191272&action=review
> > > > > > > 
> > > > > > > I still don't understand the need for all of this compositor-related complexity.  It seems like these handles can only be updated on the main thread, right?  Why not just paint them on top of the content?  The link highlight logic is a compositing layer so it can run animations, but I don't see any sign of animated behavior either here or in the UX specs.
> > > > > > > 
> > > > > > 
> > > > > > we have very much the same requirement as link highlight. If a selected text is animating, we want the handles to stick to the end points of the selection. Same with scrolling. If the handles are drawn on a compositing layer attached to the selected node's render layer, we do not have to constantly update the handles while scrolling. This is not shown in the UX specs.. but you can see the problem on chrome for android (bring up selection handles then scroll the page. you will notice the handles lagging behind briefly before they disappear). The chrome for android team also wants to move the drawing logic to the compositor (associated bug: https://code.google.com/p/chromium/issues/detail?id=135959)
> > > > > 
> > > > > You don't if the content itself is moving.  You only need a separately compositing layer if you want the selection handles to move or change appearance _independently_ of the content they are associated with (i.e. you want to fade them out).  Otherwise you just need to paint them on top of the content and are done.  Since you have to be on the main thread to create/destroy/otherwise manipulate these anyway and they should be super cheap to paint, this doesn't seem like a problem.
> > > > 
> > > > Hi James, there are a few problems with drawing on the content itself:
> > > > 1. the handles will be clipped to the clip of the layer that the content is on
> > > 
> > > Hmm, so you want a different clipping hierarchy for the handles?  That will need some additional work (beyond what you've done here) to figure out what the correct clip is and how to produce that clipping hierarchy.  This could be a lot of work depending on what it needs to do.  What is the exact clipping behavior you want?  Where are the tests for this behavior?
> > > 
> > > The link highlight system, which it appears you are mimicing, doesn't need to solve this problem since it has the same clipping requirements as the link itself.
> > 
> > Currently the handles will be clipped to the parent compositing layer's clip (if it has one). I think this works well because only layers such as scrollable divs, iframes etc. have a clip and we want the handles to respect that clip (if the selected text is not visible, we dont want the handles to be visible either). Changing the clip hierarchy seems to be very hard and I am not sure we should do that.
> 
> Do you have specific examples?

Scrollable divs and iframes are all I can think of for the moment. For instance, if you try the following iframe: data:text/html,<iframe src="http://www.bikeshed.com"></iframe>
Select some text from the iframe and then scroll so that selected text goes out of the view. The handles should also scroll out of the view in this case.

> 
> > > > 3. In future, I do plan to fade the handles in/out
> > > 
> > > It'll be really helpful if you can fully describe the behavior you want up front so we can make sure we're trying to solve the same problem.
> > > 
> > 
> > Sorry for misguiding you. We do have the fade in/out requirement. However, I am curious to know what is so bad about drawing these as a separate layer instead of on the content. From the top of it, it seems more efficient to have a separate layer to avoid redrawing (even the small portion under the handle) the content layer. More so because these layers will be added only when there is selection on the page and the user is interacting using touch. Otherwise, the layers do not add to anything performance-wise. But maybe I am missing something important here.
> > In any case, I think the fade requirement makes a strong case for separating the layer. If you agree, I can go ahead with this approach and attend to the other comments you gave earlier.
> 
> My concern is having separate manually managed layers is very complicated and will require a lot of careful work to get correct and maintain, as evidenced by all the math in this patch and the tests that will need to be written.  I'd like to make sure we know exactly what UX we are implementing and have all of the requirements and constraints in mind before committing on a specific path.

I would have thought drawing the handle on the content layer would be equally, if not more, hard (actually I dont even know of the top of my head how to do that). The UX spec is pretty much the mocks that I sent you over IM. The fade in/out is not specified in the mocks but was verbally communicated to me. You will also notice that chrome for android currently fades the handles in/out. The spec. also does not say anything specific about clipping behavior, so I assume we should do whatever makes sense.

[James Robinson]

Where do the handles go on this example: data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<span>abcdefh<span>%D7%97%D7%A8%D7%95%D7%A8%D7%9D.%202007%D7%9C%D7%98%D7%A2%D7%A0%D7%AA%D7%9D<%2Fspan>qrstuvwxyz<%2Fspan>

?

[Varun Jain]

Created attachment 191819 [details]
Example of selection handles on multi-directinoal text

[Varun Jain]

(In reply to comment #40)
> Where do the handles go on this example: data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<span>abcdefh<span>%D7%97%D7%A8%D7%95%D7%A8%D7%9D.%202007%D7%9C%D7%98%D7%A2%D7%A0%D7%AA%D7%9D<%2Fspan>qrstuvwxyz<%2Fspan>
> 
> ?

Regardless of rtl text, the selection handles are always at the logical end points of the selection. So even if the selection is spread across multiple disjoint rectangles, there will be only two handles marking the end points. See attached image (https://bugs.webkit.org/attachment.cgi?id=191819) for how it looks like.

[Varun Jain]

moved to https://code.google.com/p/chromium/issues/detail?id=115237

[Zan Dobersek]

Comment on attachment 191272 [details]
Patch

The work on the bug has ceased, removing the r? flag on this patch.