Plumb selection bounds as a single unit

Source/core/frame/FrameView.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 26 matching lines @@
 #include "core/editing/FrameSelection.h"
 #include "core/editing/RenderedPosition.h"
 #include "core/events/OverflowEvent.h"
 #include "core/fetch/ResourceFetcher.h"
 #include "core/fetch/ResourceLoadPriorityOptimizer.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/LocalFrame.h"
 #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"
 #include "core/layout/Layer.h"
 #include "core/layout/LayoutCounter.h"
 #include "core/layout/LayoutEmbeddedObject.h"
 #include "core/layout/LayoutListBox.h"
 #include "core/layout/LayoutPart.h"
 #include "core/layout/LayoutScrollbar.h"
 #include "core/layout/LayoutScrollbarPart.h"
 #include "core/layout/LayoutTheme.h"
 #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"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/Chrome.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/EventHandler.h"
 #include "core/page/FocusController.h"
 #include "core/page/FrameTree.h"
@@ skipping 1566 matching lines @@
     // If there fixed position elements, scrolling may cause compositing layers to change.
     // Update widget and layer positions after scrolling, but only if we're not inside of
     // layout.
     if (!m_nestedLayoutCount) {
         updateWidgetPositions();
         if (LayoutView* layoutView = this->layoutView())
             layoutView->layer()->setNeedsCompositingInputsUpdate();
     }
 }
 
-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())
         return false;
 
     VisiblePosition visibleStart(selection.visibleStart());
     VisiblePosition visibleEnd(selection.visibleEnd());
 
     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;
 }
 
 void FrameView::updateCompositedSelectionBoundsIfNeeded()
 {
     if (!RuntimeEnabledFeatures::compositedSelectionUpdateEnabled())
         return;
 
     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
 {
     Page* page = frame().page();
     if (!page)
         return nullptr;
     return &page->chrome();
 }
 
@@ skipping 2321 matching lines @@
 {
     Settings* settings = frame().settings();
     if (!settings || !settings->rootLayerScrolls())
         return this;
 
     LayoutView* layoutView = this->layoutView();
     return layoutView ? layoutView->scrollableArea() : nullptr;
 }
 
 } // namespace blink