Bug 1617342 - Don't treat scrollable boxes inside input elements as focusable. r=bzbarsky
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 27 Feb 2020 18:44:33 +0000
changeset 515961 ff3125134c9355727730c53561d2b29144270e25
parent 515960 fc45c8fbbb31dba9e42ccfd5df18a61a65b10c38
child 515962 a1d40d1d794fb175cc7a0cbaf11583c6be27f6a2
push id37165
push useraiakab@mozilla.com
push dateFri, 28 Feb 2020 09:24:28 +0000
treeherdermozilla-central@fb4f281c1c54 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1617342, 981248
milestone75.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1617342 - Don't treat scrollable boxes inside input elements as focusable. r=bzbarsky The code in nsIFrame::IsFocusable works for non-number input elements, because of the `IsRootOfNativeAnonymousSubtree` check, but doesn't work for `<input type=number>`, as the scrollable area is wrapped in a flex container (see nsNumberControlFrame::CreateAnonymousContent). It used to work (kinda) before bug 981248 because of the weird focus-manager redirection code to the inner <input> that was causing problems and which that bug removed. Make the check a bit more explicit, and add a test. Differential Revision: https://phabricator.services.mozilla.com/D64432
dom/base/test/mochitest.ini
dom/base/test/test_focus_scrollable_input.html
layout/generic/nsFrame.cpp
layout/generic/nsGfxScrollFrame.h
layout/generic/nsIScrollableFrame.h
--- a/dom/base/test/mochitest.ini
+++ b/dom/base/test/mochitest.ini
@@ -661,16 +661,17 @@ skip-if = toolkit == 'android' && !is_fe
 [test_find.html]
 skip-if = (toolkit == 'android') # Android: Bug 1465387
 [test_find_nac.html]
 skip-if = (toolkit == 'android') # Android: Bug 1465387
 [test_find_bug1601118.html]
 skip-if = (toolkit == 'android') # Android: Bug 1465387
 [test_focus_shadow_dom_root.html]
 [test_focus_shadow_dom.html]
+[test_focus_scrollable_input.html]
 [test_getAttribute_after_createAttribute.html]
 [test_getElementById.html]
 [test_getTranslationNodes.html]
 [test_getTranslationNodes_limit.html]
 [test_gsp-qualified.html]
 [test_gsp-quirks.html]
 [test_gsp-standards.html]
 [test_history_document_open.html]
new file mode 100644
--- /dev/null
+++ b/dom/base/test/test_focus_scrollable_input.html
@@ -0,0 +1,56 @@
+<!doctype html>
+<title>Test for bug 1617342</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<style>
+  input {
+    /* Make them small enough that any value overflows */
+    height: 10px;
+    width: 10px;
+    box-sizing: border-box;
+    font: 16px/1 monospace;
+  }
+</style>
+<input type="text" id="start" value="aaaaaaaaaa">
+
+<input type="text" value="aaaaaaaaaa">
+<input type="number" value="1111">
+<input type="search" value="aaaaaaa">
+<input type="url" value="https://fooooooooooo">
+
+<input type="text" id="end" value="aaaaaaaaaa">
+<script>
+  SimpleTest.waitForExplicitFinish();
+  SimpleTest.waitForFocus(async function() {
+    // Enable Full Keyboard Access emulation on Mac.
+    if (navigator.platform.indexOf("Mac") === 0) {
+      await SpecialPowers.pushPrefEnv({"set": [["accessibility.tabfocus", 7]]});
+    }
+
+    const start = document.getElementById("start");
+
+    start.focus();
+
+    const end = document.getElementById("end");
+
+    is(document.activeElement, start, "Focus moved sanely");
+
+    let lastActiveElement = start;
+    let stack = [start];
+
+    do {
+      synthesizeKey("KEY_Tab");
+      isnot(document.activeElement, lastActiveElement, "Focus should've moved once per tab keypress");
+      lastActiveElement = document.activeElement;
+      stack.push(lastActiveElement);
+    } while (document.activeElement != end)
+
+    do {
+      let previous = stack.pop();
+      is(document.activeElement, previous, "Focus should've moved backwards as expected");
+      synthesizeKey("KEY_Tab", {shiftKey: true});
+    } while (stack.length);
+
+    SimpleTest.finish();
+  });
+</script>
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -10190,16 +10190,17 @@ bool nsIFrame::IsFocusable(int32_t* aTab
       // Otherwise you couldn't scroll them with keyboard, which is
       // an accessibility issue (e.g. Section 508 rules)
       // However, we don't make them to be focusable with the mouse,
       // because the extra focus outlines are considered unnecessarily ugly.
       // When clicked on, the selection position within the element
       // will be enough to make them keyboard scrollable.
       nsIScrollableFrame* scrollFrame = do_QueryFrame(this);
       if (scrollFrame &&
+          !scrollFrame->IsForTextControlWithNoScrollbars() &&
           !scrollFrame->GetScrollStyles().IsHiddenInBothDirections() &&
           !scrollFrame->GetScrollRange().IsEqualEdges(nsRect(0, 0, 0, 0))) {
         // Scroll bars will be used for overflow
         isFocusable = true;
         tabIndex = 0;
       }
     }
   }
--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@@ -61,16 +61,18 @@ class ScrollFrameHelper : public nsIRefl
   class AsyncSmoothMSDScroll;
 
   ScrollFrameHelper(nsContainerFrame* aOuter, bool aIsRoot);
   ~ScrollFrameHelper();
 
   mozilla::ScrollStyles GetScrollStylesFromFrame() const;
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo() const;
 
+  bool IsForTextControlWithNoScrollbars() const;
+
   // If a child frame was added or removed on the scrollframe,
   // reload our child frame list.
   // We need this if a scrollbar frame is recreated.
   void ReloadChildFrames();
 
   nsresult CreateAnonymousContent(
       nsTArray<nsIAnonymousContentCreator::ContentInfo>& aElements);
   void AppendAnonymousContentTo(nsTArray<nsIContent*>& aElements,
@@ -723,18 +725,16 @@ class ScrollFrameHelper : public nsIRefl
 
   void CompleteAsyncScroll(const nsRect& aRange, nsAtom* aOrigin = nullptr);
 
   bool HasPluginFrames();
   bool HasPerspective() const { return mOuter->ChildrenHavePerspective(); }
   bool HasBgAttachmentLocal() const;
   mozilla::StyleDirection GetScrolledFrameDir() const;
 
-  bool IsForTextControlWithNoScrollbars() const;
-
   // Ask APZ to smooth scroll to |aDestination|.
   // This method does not clamp the destination; callers should clamp it to
   // either the layout or the visual scroll range (APZ will happily smooth
   // scroll to either).
   void ApzSmoothScrollTo(const nsPoint& aDestination, nsAtom* aOrigin);
 
   // Removes any RefreshDriver observers we might have registered.
   void RemoveObservers();
@@ -857,16 +857,19 @@ class nsHTMLScrollFrame : public nsConta
 
   // nsIScrollableFrame
   nsIFrame* GetScrolledFrame() const final {
     return mHelper.GetScrolledFrame();
   }
   mozilla::ScrollStyles GetScrollStyles() const override {
     return mHelper.GetScrollStylesFromFrame();
   }
+  bool IsForTextControlWithNoScrollbars() const final {
+    return mHelper.IsForTextControlWithNoScrollbars();
+  }
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const final {
     return mHelper.GetOverscrollBehaviorInfo();
   }
   uint32_t GetAvailableScrollingDirectionsForUserInputEvents() const final {
     return mHelper.GetAvailableScrollingDirectionsForUserInputEvents();
   }
   uint32_t GetScrollbarVisibility() const final {
@@ -1325,16 +1328,19 @@ class nsXULScrollFrame final : public ns
 
   // nsIScrollableFrame
   nsIFrame* GetScrolledFrame() const final {
     return mHelper.GetScrolledFrame();
   }
   mozilla::ScrollStyles GetScrollStyles() const final {
     return mHelper.GetScrollStylesFromFrame();
   }
+  bool IsForTextControlWithNoScrollbars() const final {
+    return mHelper.IsForTextControlWithNoScrollbars();
+  }
   mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const final {
     return mHelper.GetOverscrollBehaviorInfo();
   }
   uint32_t GetAvailableScrollingDirectionsForUserInputEvents() const final {
     return mHelper.GetAvailableScrollingDirectionsForUserInputEvents();
   }
   uint32_t GetScrollbarVisibility() const final {
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@@ -72,16 +72,22 @@ class nsIScrollableFrame : public nsIScr
    * this frame.
    *
    * This is special because they can be propagated from the <body> element,
    * unlike other styles.
    */
   virtual mozilla::ScrollStyles GetScrollStyles() const = 0;
 
   /**
+   * Returns whether this scroll frame is for a text control element with no
+   * scrollbars (for <input>, basically).
+   */
+  virtual bool IsForTextControlWithNoScrollbars() const = 0;
+
+  /**
    * Get the overscroll-behavior styles.
    */
   virtual mozilla::layers::OverscrollBehaviorInfo GetOverscrollBehaviorInfo()
       const = 0;
 
   enum { HORIZONTAL = 0x01, VERTICAL = 0x02 };
   /**
    * Return the scrollbars which are visible. It's OK to call this during reflow