Some test cases in accessibility/mac/selection-notification-focus-change.html fail
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Feb 2018 23:11:13 +0000 (23:11 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 1 Feb 2018 23:11:13 +0000 (23:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182212
<rdar://problem/36937147>

Reviewed by Antti Koivisto and Wenson Hsieh.

Source/WebCore:

The failure was caused by the async update of the selection appearance not preserving selection reveal intent.
Fixed the bug by storing the intent in a member variable and using it later.

* dom/Element.cpp:
(WebCore::Element::focus): Removed an unnecessary synchronous layout update.
* editing/FrameSelection.cpp:
(WebCore::FrameSelection::setNeedsSelectionUpdate): Use the default intent to preserve the old behavior.
(WebCore::FrameSelection::respondToNodeModification): Ditto.
(WebCore::FrameSelection::setSelection): Save the selection reveal intent.
(WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange): Use the saved intent.
* editing/FrameSelection.h:
* page/FocusController.cpp:
(WebCore::FocusController::advanceFocusDirectionally): Always update the layout before invoking
nodeRectInAbsoluteCoordinates.

LayoutTests:

Updated and rebaselined the tests.

* accessibility/ios-simulator/header-elements.html: Force the layout after each call to element.focus
now that element.focus no longer updates the layout synchronously. Ordinarily, this will happen next time
the layout is updated for paint, by JS API, etc... but we have to force the accessibility tree to be
up-to-date for testing purposes.
* accessibility/ios-simulator/table-cell-for-row-col.html: Ditto.
* accessibility/mac/selection-notification-focus-change-expected.txt: Now all the test cases are passing.
* accessibility/mac/table-with-row-col-of-headers.html: Force the layout after each call to element.focus.
* accessibility/mac/table-with-zebra-rows.html: Ditto.
* accessibility/scroll-to-global-point-main-window.html: Ditto.
* accessibility/scroll-to-make-visible-with-subfocus.html: Ditto.
* editing/input/caret-at-the-edge-of-input.html: Wait for the focused element to reveal itself by a timer.
* fast/forms/input-text-scroll-left-on-blur.html: Ditto.
* fast/forms/textarea-no-scroll-on-blur.html: Ditto.
* fast/forms/textarea-scrolled-type.html: Ditto.
* platform/mac-wk2/accessibility/mac/selection-notification-focus-change-expected.txt: Rebaselined. We now
get one less AXTextSelectionChangedFocus notification because selection updates are now coalesced as expected.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227983 268f45cc-cd09-0410-ab3c-d52691b4dbfc

18 files changed:
LayoutTests/ChangeLog
LayoutTests/accessibility/ios-simulator/header-elements.html
LayoutTests/accessibility/ios-simulator/table-cell-for-row-col.html
LayoutTests/accessibility/mac/selection-notification-focus-change-expected.txt
LayoutTests/accessibility/mac/table-with-row-col-of-headers.html
LayoutTests/accessibility/mac/table-with-zebra-rows.html
LayoutTests/accessibility/scroll-to-global-point-main-window.html
LayoutTests/accessibility/scroll-to-make-visible-with-subfocus.html
LayoutTests/editing/input/caret-at-the-edge-of-input.html
LayoutTests/fast/forms/input-text-scroll-left-on-blur.html
LayoutTests/fast/forms/textarea-no-scroll-on-blur.html
LayoutTests/fast/forms/textarea-scrolled-type.html
LayoutTests/platform/mac-wk2/accessibility/mac/selection-notification-focus-change-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/editing/FrameSelection.cpp
Source/WebCore/editing/FrameSelection.h
Source/WebCore/page/FocusController.cpp

index f2910eb..8dda30e 100644 (file)
@@ -1,3 +1,30 @@
+2018-02-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Some test cases in accessibility/mac/selection-notification-focus-change.html fail
+        https://bugs.webkit.org/show_bug.cgi?id=182212
+        <rdar://problem/36937147>
+
+        Reviewed by Antti Koivisto and Wenson Hsieh.
+
+        Updated and rebaselined the tests.
+
+        * accessibility/ios-simulator/header-elements.html: Force the layout after each call to element.focus
+        now that element.focus no longer updates the layout synchronously. Ordinarily, this will happen next time
+        the layout is updated for paint, by JS API, etc... but we have to force the accessibility tree to be
+        up-to-date for testing purposes.
+        * accessibility/ios-simulator/table-cell-for-row-col.html: Ditto.
+        * accessibility/mac/selection-notification-focus-change-expected.txt: Now all the test cases are passing.
+        * accessibility/mac/table-with-row-col-of-headers.html: Force the layout after each call to element.focus.
+        * accessibility/mac/table-with-zebra-rows.html: Ditto.
+        * accessibility/scroll-to-global-point-main-window.html: Ditto.
+        * accessibility/scroll-to-make-visible-with-subfocus.html: Ditto.
+        * editing/input/caret-at-the-edge-of-input.html: Wait for the focused element to reveal itself by a timer.
+        * fast/forms/input-text-scroll-left-on-blur.html: Ditto.
+        * fast/forms/textarea-no-scroll-on-blur.html: Ditto.
+        * fast/forms/textarea-scrolled-type.html: Ditto.
+        * platform/mac-wk2/accessibility/mac/selection-notification-focus-change-expected.txt: Rebaselined. We now
+        get one less AXTextSelectionChangedFocus notification because selection updates are now coalesced as expected.
+
 2018-02-01  Antoine Quint  <graouts@apple.com>
 
         [Modern Media Controls] Turn media/modern-media-controls/ios-inline-media-controls back on
index e525d9d..7763d59 100644 (file)
@@ -25,9 +25,11 @@ if (window.testRunner)
     if (window.accessibilityController) {
 
         document.getElementById("button").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var button = accessibilityController.focusedElement;
 
         document.getElementById("header").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var header = accessibilityController.focusedElement;
         shouldBeTrue("button.headerElementAtIndex(0).isEqual(header)");
     }
index ff17138..2a01ca3 100644 (file)
@@ -25,14 +25,17 @@ if (window.testRunner)
     if (window.accessibilityController) {
 
         document.getElementById("cell2").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var cell2 = accessibilityController.focusedElement;
 
         document.getElementById("header").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var header = accessibilityController.focusedElement;
 
         shouldBeTrue("cell2.cellForColumnAndRow(2, 0).isEqual(header)");
 
         document.getElementById("cell5").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var cell6 = accessibilityController.focusedElement;
         shouldBeTrue("header.cellForColumnAndRow(1, 2).isEqual(cell6)");
 
index 6f98ef8..96cb28f 100644 (file)
@@ -18,7 +18,7 @@ Received AXFocusChanged
 eventSender.keyDown(tabCharacter)
 Received AXFocusChanged
 Received AXSelectedTextChanged
-FAIL userInfo["AXTextSelectionChangedFocus"] should be true (of type boolean). Was undefined (of type undefined).
+PASS userInfo["AXTextSelectionChangedFocus"] is true
 Received AXSelectedTextChanged
 PASS userInfo["AXTextSelectionChangedFocus"] is true
 PASS successfullyParsed is true
index 0a80957..440e6da 100644 (file)
     description("This tests that tables that have THs in the first row or first column will be exposed as AXTables.");
 
     if (window.accessibilityController) {
+        document.getElementById("table1").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+        shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
 
-          document.getElementById("table1").focus();
-          shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
+        document.getElementById("table2").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+        shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
 
-          document.getElementById("table2").focus();
-          shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
+        document.getElementById("table3").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+        shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXGroup'");
 
-          document.getElementById("table3").focus();
-          shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXGroup'");
-
-          document.getElementById("table4").focus();
-          shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXGroup'");
+        document.getElementById("table4").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+        shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXGroup'");
     }
 
 </script>
index 14dd2b7..ff660e1 100644 (file)
@@ -24,9 +24,9 @@
     description("This tests that tables with zebra striped rows are exposed as AXTables.");
 
     if (window.accessibilityController) {
-
-          document.getElementById("table1").focus();
-          shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
+        document.getElementById("table1").focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+        shouldBe("accessibilityController.focusedElement.role", "'AXRole: AXTable'");
     }
 
 </script>
index 4e8f491..cc43352 100644 (file)
@@ -25,6 +25,7 @@ function runTest() {
 
     if (window.accessibilityController) {
         target.focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         var targetAccessibleObject = accessibilityController.focusedElement;
     }
 
index ccd0745..fd8d414 100644 (file)
@@ -20,6 +20,7 @@ function runTest() {
     var targetAccessibleObject;
     if (window.accessibilityController) {
         target.focus();
+        internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
         targetAccessibleObject = accessibilityController.focusedElement;
     }
 
index 0a5ef59..8d698d8 100644 (file)
@@ -6,14 +6,21 @@
 <input type="text" name="input-edit" id="input-edit" size="10" value="012345678901234567890123456789" />
 <script>
 
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
 var inputEdit = document.getElementById("input-edit");
 inputEdit.focus();
-var inputEditContent = inputEdit.value;
-inputEdit.setSelectionRange(0, 0);
-if (window.eventSender) {
-    for (var i = 0; i < inputEdit.size * 1.2; ++i)
-        eventSender.keyDown(inputEditContent[i]);
-}
+setTimeout(() => {
+    var inputEditContent = inputEdit.value;
+    inputEdit.setSelectionRange(0, 0);
+
+    if (window.eventSender) {
+        for (var i = 0; i < inputEdit.size * 1.2; ++i)
+            eventSender.keyDown(inputEditContent[i]);
+        testRunner.notifyDone();
+    }
+}, 0);
 
 </script>
 </body>
index 6abfeb1..75efe34 100644 (file)
@@ -3,22 +3,46 @@
 <input id="c" type="text" value="this text field has a lot of text in it so that it needs to scrol">
 <p>Tests scrolling back to the beginning when a text field blurs. The first field should be scrolled to the left, the second and third scrolled to the right.</p>
 <script>
-var a = document.getElementById("a");
-a.focus();
-a.setSelectionRange(66, 66);
-if (window.eventSender) {
-    eventSender.keyDown("l");
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+function wait(seconds)
+{
+    return new Promise((resolve) => setTimeout(resolve, seconds))
 }
-var b = document.getElementById("b");
-b.focus();
-b.setSelectionRange(66, 66);
-if (window.eventSender) {
-    eventSender.keyDown("l");
-}
-var c = document.getElementById("c");
-c.focus();
-c.setSelectionRange(66, 66);
-if (window.eventSender) {
-    eventSender.keyDown("l");
+
+async function runTest()
+{
+    if (!window.eventSender) {
+        document.body.innerHTML += 'This test requires eventSender.keyDown';
+        return;
+    }
+
+    var a = document.getElementById("a");
+    a.focus();
+    await wait(0);
+    a.setSelectionRange(66, 66);
+    if (window.eventSender)
+        eventSender.keyDown("l");
+
+    var b = document.getElementById("b");
+    b.focus();
+    await wait(0);
+    b.setSelectionRange(66, 66);
+    if (window.eventSender)
+        eventSender.keyDown("l");
+
+    var c = document.getElementById("c");
+    c.focus();
+    await wait(0);
+    c.setSelectionRange(66, 66);
+    if (window.eventSender)
+        eventSender.keyDown("l");
+
+    if (window.testRunner)
+        testRunner.notifyDone();
 }
+
+window.onload = runTest;
+
 </script>
index f80ec90..6e7d6c1 100644 (file)
@@ -1,21 +1,34 @@
 <html>
 <head>
 <script>
-    function test() {
-        if (window.testRunner)
+    function wait(seconds) {
+        return new Promise((resolve) => setTimeout(resolve, 0), seconds);
+    }
+
+    async function test() {
+        if (window.testRunner) {
             testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
 
         var ta = document.getElementById('ta');
 
         // Send caret to bottom of textarea
         ta.focus();
         ta.setSelectionRange(ta.value.length, ta.value.length);
+        await wait(0);
         ta.blur();
 
+        await wait(0);
         ta.focus();
+        await wait(0);
         ta.blur();
-        if (ta.scrollTop != 0)
-            document.getElementById('res').innerHTML = "Test Passed";
+        await wait(0);
+
+        document.getElementById('res').innerHTML = ta.scrollTop != 0 ? "Test Passed" : "Test Failed";
+
+        if (window.testRunner)
+            testRunner.notifyDone();
     }
 </script>
 </head>
@@ -29,6 +42,6 @@ This tests that we don't scroll back to the top when leaving a textarea
 4
 5
 </textarea>
-<div id="res">Test Failed</div>
+<div id="res"></div>
 </body>
 </html>
index 90d8a39..96d5420 100644 (file)
@@ -2,17 +2,29 @@
 <head>
 <script src=../../editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
 <script>
-function test()
+function wait(seconds)
 {
+    return new Promise((resolve) => setTimeout(resolve, 0), seconds);
+}
+
+async function test()
+{
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
     var res = document.getElementById('res');
     var ta = document.getElementById('ta');
 
     // Send caret to bottom of textarea
     ta.focus();
+    await wait(0);
     ta.setSelectionRange(ta.value.length, ta.value.length);
+    await wait(0);
     ta.blur();
+    await wait(0);
 
     ta.focus();
+    await wait(0);
     ta.setSelectionRange(44, 44);
     typeCharacterCommand(' ');
     typeCharacterCommand('P');
@@ -20,6 +32,8 @@ function test()
     typeCharacterCommand('s');
     typeCharacterCommand('s');
 
+    if (window.testRunner)
+        testRunner.notifyDone();
 }
 </script>
 </head>
index 3c68f92..7211eb0 100644 (file)
@@ -18,9 +18,7 @@ Received AXFocusChanged
 eventSender.keyDown(tabCharacter)
 Received AXFocusChanged
 Received AXSelectedTextChanged
-FAIL userInfo["AXTextSelectionChangedFocus"] should be true (of type boolean). Was undefined (of type undefined).
-Received AXSelectedTextChanged
-FAIL userInfo["AXTextSelectionChangedFocus"] should be true (of type boolean). Was undefined (of type undefined).
+PASS userInfo["AXTextSelectionChangedFocus"] is true
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 828438d..59ad9c1 100644 (file)
@@ -1,3 +1,26 @@
+2018-02-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Some test cases in accessibility/mac/selection-notification-focus-change.html fail
+        https://bugs.webkit.org/show_bug.cgi?id=182212
+        <rdar://problem/36937147>
+
+        Reviewed by Antti Koivisto and Wenson Hsieh.
+
+        The failure was caused by the async update of the selection appearance not preserving selection reveal intent.
+        Fixed the bug by storing the intent in a member variable and using it later.
+
+        * dom/Element.cpp:
+        (WebCore::Element::focus): Removed an unnecessary synchronous layout update.
+        * editing/FrameSelection.cpp:
+        (WebCore::FrameSelection::setNeedsSelectionUpdate): Use the default intent to preserve the old behavior.
+        (WebCore::FrameSelection::respondToNodeModification): Ditto.
+        (WebCore::FrameSelection::setSelection): Save the selection reveal intent.
+        (WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange): Use the saved intent.
+        * editing/FrameSelection.h:
+        * page/FocusController.cpp:
+        (WebCore::FocusController::advanceFocusDirectionally): Always update the layout before invoking
+        nodeRectInAbsoluteCoordinates.
+
 2018-02-01  Zalan Bujtas  <zalan@apple.com>
 
         [RenderTreeBuilder] Move RenderRubyRun::rubyBaseSafe to RenderTreeBuilder::Ruby
index c363ace..549466b 100644 (file)
@@ -2423,9 +2423,6 @@ void Element::focus(bool restorePreviousSelection, FocusDirection direction)
             return;
     }
 
-    // Setting the focused node above might have invalidated the layout due to scripts.
-    document().updateLayoutIgnorePendingStylesheets();
-
     SelectionRevealMode revealMode = SelectionRevealMode::Reveal;
 #if PLATFORM(IOS)
     // Focusing a form element triggers animation in UIKit to scroll to the right position.
index 400edb9..bef4dc9 100644 (file)
@@ -374,6 +374,7 @@ void FrameSelection::setSelection(const VisibleSelection& selection, SetSelectio
         m_selectionRevealMode = SelectionRevealMode::DoNotReveal;
     m_alwaysAlignCursorOnScrollWhenRevealingSelection = align == AlignCursorOnScrollAlways;
 
+    m_selectionRevealIntent = intent;
     m_pendingSelectionUpdate = true;
 
     if (document->hasPendingStyleRecalc())
@@ -402,6 +403,7 @@ static void updateSelectionByUpdatingLayoutOrStyle(Frame& frame)
 
 void FrameSelection::setNeedsSelectionUpdate()
 {
+    m_selectionRevealIntent = AXTextStateChangeIntent();
     m_pendingSelectionUpdate = true;
     if (RenderView* view = m_frame->contentRenderer())
         view->selection().clear();
@@ -526,6 +528,7 @@ void FrameSelection::respondToNodeModification(Node& node, bool baseRemoved, boo
             renderView->selection().clear();
 
             // Trigger a selection update so the selection will be set again.
+            m_selectionRevealIntent = AXTextStateChangeIntent();
             m_pendingSelectionUpdate = true;
             renderView->frameView().scheduleSelectionUpdate();
         }
@@ -2447,7 +2450,7 @@ void FrameSelection::updateAppearanceAfterLayoutOrStyleChange()
         client->updateEditorStateAfterLayoutIfEditabilityChanged();
 
     setCaretRectNeedsUpdate();
-    updateAndRevealSelection(AXTextStateChangeIntent());
+    updateAndRevealSelection(m_selectionRevealIntent);
     updateDataDetectorsForSelection();
 }
 
index 79e1717..0b472be 100644 (file)
@@ -347,6 +347,7 @@ private:
     IntRect m_absCaretBounds;
 
     SelectionRevealMode m_selectionRevealMode { SelectionRevealMode::DoNotReveal };
+    AXTextStateChangeIntent m_selectionRevealIntent;
 
     bool m_caretInsidePositionFixed : 1;
     bool m_absCaretBoundsDirty : 1;
index bc1818f..5e4efb0 100644 (file)
@@ -1081,8 +1081,7 @@ bool FocusController::advanceFocusDirectionally(FocusDirection direction, Keyboa
     Element* focusedElement = focusedDocument->focusedElement();
     Node* container = focusedDocument;
 
-    if (is<Document>(*container))
-        downcast<Document>(*container).updateLayoutIgnorePendingStylesheets();
+    focusedDocument->updateLayoutIgnorePendingStylesheets();
 
     // Figure out the starting rect.
     LayoutRect startingRect;
@@ -1103,10 +1102,9 @@ bool FocusController::advanceFocusDirectionally(FocusDirection direction, Keyboa
     bool consumed = false;
     do {
         consumed = advanceFocusDirectionallyInContainer(container, startingRect, direction, event);
+        focusedDocument->updateLayoutIgnorePendingStylesheets();
         startingRect = nodeRectInAbsoluteCoordinates(container, true /* ignore border */);
         container = scrollableEnclosingBoxOrParentFrameForNodeInDirection(direction, container);
-        if (is<Document>(container))
-            downcast<Document>(*container).updateLayoutIgnorePendingStylesheets();
     } while (!consumed && container);
 
     return consumed;