Changeset 164156 in webkit

Timestamp:

Feb 14, 2014, 8:52:38 PM (12 years ago)

Author:

rniwa@webkit.org

Message:

setSelectionRange shouldn't trigger a synchronous layout to check focusability when text field is already focused
​https://bugs.webkit.org/show_bug.cgi?id=128804

Reviewed by Enrica Casucci.

Don't trigger a synchronous layout at the beginning of setSelectionRange if the element is already focused
since we don't have to check the size of render box in that case.

We should be able to get rid of this synchronous layout entirely once we fix ​https://webkit.org/b/128797
but that's somewhat risky behavioral change so we'll do that in a separate patch.

• editing/FrameSelection.cpp:

(WebCore::FrameSelection::selectAll): Fixed the bug where selectAll selects the entire document even if the text
form contol is focused if the selection is none (i.e. not anchored to any node).

• html/HTMLTextFormControlElement.cpp:

(WebCore::HTMLTextFormControlElement::setSelectionRange): Only update the layout if the element is not focused
already. Also pass in DoNotSetFocus option to setSelection since we already have the focus in that case.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• editing/FrameSelection.cpp (modified) (2 diffs)
• html/HTMLTextFormControlElement.cpp (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-02-14  Ryosuke Niwa  <rniwa@webkit.org>
+
+        setSelectionRange shouldn't trigger a synchronous layout to check focusability when text field is already focused
+        https://bugs.webkit.org/show_bug.cgi?id=128804
+
+        Reviewed by Enrica Casucci.
+
+        Don't trigger a synchronous layout at the beginning of setSelectionRange if the element is already focused
+        since we don't have to check the size of render box in that case.
+
+        We should be able to get rid of this synchronous layout entirely once we fix https://webkit.org/b/128797
+        but that's somewhat risky behavioral change so we'll do that in a separate patch.
+
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::selectAll): Fixed the bug where selectAll selects the entire document even if the text
+        form contol is focused if the selection is none (i.e. not anchored to any node).
+        * html/HTMLTextFormControlElement.cpp:
+        (WebCore::HTMLTextFormControlElement::setSelectionRange): Only update the layout if the element is not focused
+        already. Also pass in DoNotSetFocus option to setSelection since we already have the focus in that case.
+
 2014-02-14  Dan Bernstein  <mitz@apple.com>
 

trunk/Source/WebCore/editing/FrameSelection.cpp

@@ -1641 +1641 @@
     Document* document = m_frame->document();
 
-    if (document->focusedElement() && document->focusedElement()->hasTagName(selectTag)) {
+    Element* focusedElement = document->focusedElement();
+    if (focusedElement && focusedElement->hasTagName(selectTag)) {
         HTMLSelectElement* selectElement = toHTMLSelectElement(document->focusedElement());
         if (selectElement->canSelectAll()) {
@@ -1658 +1659 @@
             selectStartTarget = root.get();
     } else {
-        root = m_selection.nonBoundaryShadowTreeRootNode();
+        if (m_selection.isNone() && focusedElement) {
+            if (focusedElement->isTextFormControl()) {
+                toHTMLTextFormControlElement(focusedElement)->select();
+                return;
+            }
+            root = focusedElement->nonBoundaryShadowTreeRootNode();
+        } else
+            root = m_selection.nonBoundaryShadowTreeRootNode();
+
         if (root)
             selectStartTarget = root->shadowHost();

trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp

@@ -214 +214 @@
 }
 
-static inline bool hasVisibleTextArea(RenderElement& textControl, TextControlInnerTextElement* innerText)
-{
-    return textControl.style().visibility() != HIDDEN && innerText && innerText->renderer() && innerText->renderBox()->height();
-}
-
 void HTMLTextFormControlElement::setRangeText(const String& replacement, ExceptionCode& ec)
 {
@@ -291 +286 @@
 void HTMLTextFormControlElement::setSelectionRange(int start, int end, TextFieldSelectionDirection direction)
 {
-    document().updateLayoutIgnorePendingStylesheets();
-
-    if (!renderer() || !renderer()->isTextControl())
+    if (!isTextFormControl())
         return;
 
@@ -300 +293 @@
 
     TextControlInnerTextElement* innerText = innerTextElement();
-    if (!hasVisibleTextArea(*renderer(), innerText)) {
-        cacheSelection(start, end, direction);
-        return;
-    }
+    bool hasFocus = document().focusedElement() == this;
+    if (!hasFocus && innerText) {
+        // FIXME: Removing this synchronous layout requires fixing <https://webkit.org/b/128797>
+        document().updateLayoutIgnorePendingStylesheets();
+        if (RenderElement* rendererTextControl = renderer()) {
+            if (rendererTextControl->style().visibility() == HIDDEN || !innerText->renderBox()->height()) {
+                cacheSelection(start, end, direction);
+                return;
+            }
+        }
+    }
+
     Position startPosition = positionForIndex(innerText, start);
     Position endPosition;
@@ -318 +319 @@
     newSelection.setIsDirectional(direction != SelectionHasNoDirection);
 
+    FrameSelection::SetSelectionOptions options = FrameSelection::defaultSetSelectionOptions();
+    if (hasFocus)
+        options |= FrameSelection::DoNotSetFocus;
     if (Frame* frame = document().frame())
-        frame->selection().setSelection(newSelection);
+        frame->selection().setSelection(newSelection, options);
 }