Plumb selection bounds as a single unit

Source/core/frame/FrameView.cpp

Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index 0e19021b2ad3a32e2b34404f9f66cd7904a33655..bbb5088ad0f0a388ae93b93999f14dc71dd0b85c 100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -44,6 +44,7 @@
 #include "core/frame/Settings.h"
 #include "core/html/HTMLFrameElement.h"
 #include "core/html/HTMLPlugInElement.h"
+#include "core/html/HTMLTextFormControlElement.h"
 #include "core/html/parser/TextResourceDecoder.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/inspector/InspectorTraceEvents.h"
@@ -58,7 +59,7 @@
 #include "core/layout/LayoutView.h"
 #include "core/layout/TextAutosizer.h"
 #include "core/layout/compositing/CompositedLayerMapping.h"
-#include "core/layout/compositing/CompositedSelectionBound.h"
+#include "core/layout/compositing/CompositedSelectionBounds.h"
 #include "core/layout/compositing/LayerCompositor.h"
 #include "core/layout/style/LayoutStyle.h"
 #include "core/layout/svg/LayoutSVGRoot.h"
@@ -1645,7 +1646,7 @@ void FrameView::updateLayersAndCompositingAfterScrollIfNeeded()
     }
 }
 
-bool FrameView::computeCompositedSelectionBounds(LocalFrame& frame, CompositedSelectionBound& start, CompositedSelectionBound& end)
+bool FrameView::computeCompositedSelectionBounds(LocalFrame& frame, CompositedSelectionBounds& bounds)
 {
     const VisibleSelection &selection = frame.selection().selection();
     if (!selection.isCaretOrRange())
@@ -1657,23 +1658,29 @@ bool FrameView::computeCompositedSelectionBounds(LocalFrame& frame, CompositedSe
     RenderedPosition renderedStart(visibleStart);
     RenderedPosition renderedEnd(visibleEnd);
 
-    renderedStart.positionInGraphicsLayerBacking(start);
-    if (!start.layer)
+    renderedStart.positionInGraphicsLayerBacking(bounds.start);
+    if (!bounds.start.layer)
         return false;
 
-    renderedEnd.positionInGraphicsLayerBacking(end);
-    if (!end.layer)
+    renderedEnd.positionInGraphicsLayerBacking(bounds.end);
+    if (!bounds.end.layer)
         return false;
 
+    bounds.isEditable = selection.isContentEditable();

[Rick Byers, 2015/03/12 19:27:24]

does isContentEditable really return true for the common editable cases like
<input>?  I expected (from the name and quick glance at the code) that this
would be true only when the element or an ancestor has the contentEditable=true
attribute.

[jdduke (slow), 2015/03/16 18:15:41]

Hmm, it appears to, yes. One odd thing that I noticed is that we don't display
the OSK when we enter a "contentEditable" div, which seems like a bug?

+    if (bounds.isEditable) {
+        if (HTMLTextFormControlElement* enclosingTextFormControlElement = enclosingTextFormControl(selection.rootEditableElement()))
+            bounds.isEditableRegionEmpty = enclosingTextFormControlElement->value().isEmpty();

[jdduke (slow), 2015/03/12 18:18:38]

If there's a better way of determining this, please share :)

[Rick Byers, 2015/03/12 19:27:23]

not sure off hand.  yosin@?

Does this handle the 'empty' contentEditable=true case properly?

[yosin_UTC9, 2015/03/13 02:06:37]

Short answer is "yes". Long answer is "case by case" since word "properly" is
ambiguous.
Could you tell me your usage of result of |isContentEditable()|?
Current implementation returns true if container node of start position has
user-modify: read-write regardless editability of between start(exclusive) to
end. In this case, replacing selection content operation replaces text at start
position and rest of text are left.

P.S. It is editing team due to handl text form control editablity by
|isContentEditable()|. It is not good idea to check text form control here.

[jdduke (slow), 2015/03/16 18:15:41]

Sure. We need to know whether to display a text insertion handle if the
selection area is editable. In short, this should only be true for areas where
we can insert text, so it's possible there's a different bit we need here?
So, what property should I check?

[yosin_UTC9, 2015/03/18 05:15:41]

How about calling |isContentEdtiable()| if |selection.isCaret()|? Because of
determining editability of selection range is unclear, e.g. range contains
uneditable node.

I think you don't need to check form controls here, it should be handled by
|isContentEditable()| function, if not, please file a bug, we'll fix it.

[jdduke (slow), 2015/03/18 20:20:45]

That's one option. I think what we want is for this to align with the IME
concept of editability, i.e., if the textInputType is not "none".
The reason we want to know if the text region is empty is the following:

If the user taps an empty field, we don't want to display the insertion handle.
Otherwise, we do want to display the insertion handle. It's not a big deal, but
it's necessary to align with native Android widget behavior.

+    }
+
     if (selection.isCaret()) {
-        start.type = end.type = CompositedSelectionBound::Caret;
+        bounds.start.type = bounds.end.type = CompositedSelectionBound::Caret;
         return true;
     }
 
     TextDirection startDir = visibleStart.deepEquivalent().primaryDirection();
     TextDirection endDir = visibleEnd.deepEquivalent().primaryDirection();
-    start.type = startDir == RTL ? CompositedSelectionBound::SelectionRight : CompositedSelectionBound::SelectionLeft;
-    end.type = endDir == RTL ? CompositedSelectionBound::SelectionLeft : CompositedSelectionBound::SelectionRight;
+    bounds.start.type = startDir == RTL ? CompositedSelectionBound::SelectionRight : CompositedSelectionBound::SelectionLeft;
+    bounds.end.type = endDir == RTL ? CompositedSelectionBound::SelectionLeft : CompositedSelectionBound::SelectionRight;
     return true;
 }
 
@@ -1685,14 +1692,14 @@ void FrameView::updateCompositedSelectionBoundsIfNeeded()
     Page* page = frame().page();
     ASSERT(page);
 
-    CompositedSelectionBound start, end;
+    CompositedSelectionBounds bounds;
     LocalFrame* frame = toLocalFrame(page->focusController().focusedOrMainFrame());
-    if (!frame || !computeCompositedSelectionBounds(*frame, start, end)) {
+    if (!frame || !computeCompositedSelectionBounds(*frame, bounds)) {
         page->chrome().client().clearCompositedSelectionBounds();
         return;
     }
 
-    page->chrome().client().updateCompositedSelectionBounds(start, end);
+    page->chrome().client().updateCompositedSelectionBounds(bounds);
 }
 
 HostWindow* FrameView::hostWindow() const