Bug 1723010 - Part 2: Stop iterating to find next element for an accesskey once the accesskey has been processed; r=masayuki

even if the focus isn't changed.

Depends on D122787

Differential Revision: https://phabricator.services.mozilla.com/D122349

dom/base/Element.h

@@ -31,6 +31,7 @@
 #include "mozilla/Maybe.h"
 #include "mozilla/PseudoStyleType.h"
 #include "mozilla/RefPtr.h"
+#include "mozilla/Result.h"
 #include "mozilla/RustCell.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/dom/BorrowedAttrInfo.h"
@@ -1657,11 +1658,15 @@ class Element : public FragmentOrElement {
    * @param aKeyCausesActivation - if true then element should be activated
    * @param aIsTrustedEvent - if true then event that is cause of accesskey
    *                          execution is trusted.
-   * @return true if the focus was changed.
+   * @return an error if the element isn't able to handle the accesskey (caller
+   *         would look for the next element to handle it).
+   *         a boolean indicates whether the focus moves to the element after
+   *         the element handles the accesskey.
    */
-  MOZ_CAN_RUN_SCRIPT virtual bool PerformAccesskey(bool aKeyCausesActivation,
-                                                   bool aIsTrustedEvent) {
-    return false;
+  MOZ_CAN_RUN_SCRIPT
+  virtual Result<bool, nsresult> PerformAccesskey(bool aKeyCausesActivation,
+                                                  bool aIsTrustedEvent) {
+    return Err(NS_ERROR_NOT_IMPLEMENTED);
   }
 
  protected:

dom/events/EventStateManager.cpp

@@ -1161,8 +1161,10 @@ bool EventStateManager::LookForAccessKeyAndExecute(
           }
         }
 
-        if (element->PerformAccesskey(shouldActivate, aIsTrustedEvent)) {
-          if (aIsTrustedEvent) {
+        auto result =
+            element->PerformAccesskey(shouldActivate, aIsTrustedEvent);
+        if (result.isOk()) {
+          if (result.unwrap() && aIsTrustedEvent) {
             // If this is a child process, inform the parent that we want the
             // focus, but pass false since we don't want to change the window
             // order.

dom/events/test/test_accesskey.html

@@ -31,6 +31,10 @@
 <input type="radio" id="radio4" disabled><label for="radio4">Radio 4</label><br>
 </fieldset>
 <input type="text" id="text1" accesskey="d"><br>
+<!-- Tests for bug 1723010 -->
+<button id="button5" style="display:none" accesskey="1">Button 5</button>
+<button id="button6" style="display:none" accesskey="2">Button 6</button>
+<textarea id="textarea1" accesskey="2"></textarea>
 <script>
 
 function performAccessKey(aKey) {
@@ -79,6 +83,48 @@ add_task(function legend2() {
   is(document.activeElement.id, text1.id, `focus should move to ${text1.id}`);
 });
 
+/** Test for Bug 1723010 **/
+
+add_task(async function removeElement() {
+  let button5 = document.getElementById("button5");
+  let textarea1 = document.getElementById("textarea1");
+  let promise = new Promise((resolve) => {
+    button5.addEventListener("click", function() {
+      textarea1.remove();
+      SimpleTest.executeSoon(() => {
+        ok(true, "should not crash");
+        resolve();
+      });
+    }, { once: true });
+  });
+
+  performAccessKey("1");
+  await promise;
+});
+
+add_task(async function modifyAccessKey() {
+  let button5 = document.getElementById("button5");
+  let button6 = document.getElementById("button6");
+  let textarea1 = document.querySelector("textarea1");
+  let promise = new Promise((resolve) => {
+    button5.addEventListener("click", function() {
+      button5.setAttribute("accesskey", "2");
+      button6.setAttribute("accesskey", "1");
+      SimpleTest.executeSoon(() => {
+        ok(true, "Button 5 should be clicked");
+        resolve();
+      });
+    }, { once: true });
+
+    button6.addEventListener("click", function() {
+      ok(false, "Button 6 should not be clicked");
+    }, { once: true });
+  });
+
+  performAccessKey("1");
+  await promise;
+});
+
 </script>
 </body>
 </html>

dom/html/HTMLLabelElement.cpp

@@ -177,26 +177,28 @@ nsresult HTMLLabelElement::PostHandleEvent(EventChainPostVisitor& aVisitor) {
   return NS_OK;
 }
 
-bool HTMLLabelElement::PerformAccesskey(bool aKeyCausesActivation,
-                                        bool aIsTrustedEvent) {
+Result<bool, nsresult> HTMLLabelElement::PerformAccesskey(
+    bool aKeyCausesActivation, bool aIsTrustedEvent) {
   if (!aKeyCausesActivation) {
     RefPtr<Element> element = GetLabeledElement();
     if (element) {
       return element->PerformAccesskey(aKeyCausesActivation, aIsTrustedEvent);
     }
-  } else {
-    nsPresContext* presContext = GetPresContext(eForUncomposedDoc);
-    if (!presContext) {
-      return false;
-    }
-
-    // Click on it if the users prefs indicate to do so.
-    AutoPopupStatePusher popupStatePusher(
-        aIsTrustedEvent ? PopupBlocker::openAllowed : PopupBlocker::openAbused);
-    DispatchSimulatedClick(this, aIsTrustedEvent, presContext);
+    return Err(NS_ERROR_ABORT);
   }
 
-  return aKeyCausesActivation;
+  nsPresContext* presContext = GetPresContext(eForUncomposedDoc);
+  if (!presContext) {
+    return Err(NS_ERROR_UNEXPECTED);
+  }
+
+  // Click on it if the users prefs indicate to do so.
+  AutoPopupStatePusher popupStatePusher(
+      aIsTrustedEvent ? PopupBlocker::openAllowed : PopupBlocker::openAbused);
+  DispatchSimulatedClick(this, aIsTrustedEvent, presContext);
+
+  // XXXedgar, do we need to check whether the focus is really changed?
+  return true;
 }
 
 nsGenericHTMLElement* HTMLLabelElement::GetLabeledElement() const {

dom/html/HTMLLabelElement.h

@@ -48,7 +48,8 @@ class HTMLLabelElement final : public nsGenericHTMLElement {
   // nsIContent
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   virtual nsresult PostHandleEvent(EventChainPostVisitor& aVisitor) override;
-  MOZ_CAN_RUN_SCRIPT virtual bool PerformAccesskey(
+  MOZ_CAN_RUN_SCRIPT
+  virtual Result<bool, nsresult> PerformAccesskey(
       bool aKeyCausesActivation, bool aIsTrustedEvent) override;
   virtual nsresult Clone(dom::NodeInfo*, nsINode** aResult) const override;
 

dom/html/HTMLLegendElement.cpp

@@ -93,13 +93,18 @@ void HTMLLegendElement::Focus(const FocusOptions& aOptions,
                          getter_AddRefs(result));
 }
 
-bool HTMLLegendElement::PerformAccesskey(bool aKeyCausesActivation,
-                                         bool aIsTrustedEvent) {
+Result<bool, nsresult> HTMLLegendElement::PerformAccesskey(
+    bool aKeyCausesActivation, bool aIsTrustedEvent) {
   FocusOptions options;
   ErrorResult rv;
 
   Focus(options, CallerType::System, rv);
-  return NS_SUCCEEDED(rv.StealNSResult());
+  if (rv.Failed()) {
+    return Err(rv.StealNSResult());
+  }
+
+  // XXXedgar, do we need to check whether the focus is really changed?
+  return true;
 }
 
 HTMLLegendElement::LegendAlignValue HTMLLegendElement::LogicalAlign(

dom/html/HTMLLegendElement.h

@@ -27,8 +27,8 @@ class HTMLLegendElement final : public nsGenericHTMLElement {
                      const mozilla::dom::CallerType aCallerType,
                      ErrorResult& aError) override;
 
-  virtual bool PerformAccesskey(bool aKeyCausesActivation,
-                                bool aIsTrustedEvent) override;
+  virtual Result<bool, nsresult> PerformAccesskey(
+      bool aKeyCausesActivation, bool aIsTrustedEvent) override;
 
   // nsIContent
   virtual nsresult BindToTree(BindContext&, nsINode& aParent) override;

dom/html/nsGenericHTMLElement.cpp

@@ -2409,11 +2409,11 @@ bool nsGenericHTMLElement::IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
   return disallowOverridingFocusability;
 }
 
-bool nsGenericHTMLElement::PerformAccesskey(bool aKeyCausesActivation,
-                                            bool aIsTrustedEvent) {
+Result<bool, nsresult> nsGenericHTMLElement::PerformAccesskey(
+    bool aKeyCausesActivation, bool aIsTrustedEvent) {
   nsPresContext* presContext = GetPresContext(eForComposedDoc);
   if (!presContext) {
-    return false;
+    return Err(NS_ERROR_UNEXPECTED);
   }
 
   // It's hard to say what HTML4 wants us to do in all cases.
@@ -2431,9 +2431,13 @@ bool nsGenericHTMLElement::PerformAccesskey(bool aKeyCausesActivation,
     AutoPopupStatePusher popupStatePusher(
         aIsTrustedEvent ? PopupBlocker::openAllowed : PopupBlocker::openAbused);
     DispatchSimulatedClick(this, aIsTrustedEvent, presContext);
+    return focused;
   }
 
-  return focused;
+  // If the accesskey won't cause the activation and the focus isn't changed,
+  // either. Return error so EventStateManager would try to find next element
+  // to handle the accesskey.
+  return focused ? Result<bool, nsresult>{focused} : Err(NS_ERROR_ABORT);
 }
 
 void nsGenericHTMLElement::HandleKeyboardActivation(

dom/html/nsGenericHTMLElement.h

@@ -282,7 +282,8 @@ class nsGenericHTMLElement : public nsGenericHTMLElementBase {
    */
   virtual bool IsHTMLFocusable(bool aWithMouse, bool* aIsFocusable,
                                int32_t* aTabIndex);
-  MOZ_CAN_RUN_SCRIPT virtual bool PerformAccesskey(
+  MOZ_CAN_RUN_SCRIPT
+  virtual mozilla::Result<bool, nsresult> PerformAccesskey(
       bool aKeyCausesActivation, bool aIsTrustedEvent) override;
 
   /**

dom/xul/nsXULElement.cpp

@@ -461,25 +461,25 @@ void nsXULElement::OpenMenu(bool aOpenFlag) {
   }
 }
 
-bool nsXULElement::PerformAccesskey(bool aKeyCausesActivation,
-                                    bool aIsTrustedEvent) {
+Result<bool, nsresult> nsXULElement::PerformAccesskey(bool aKeyCausesActivation,
+                                                      bool aIsTrustedEvent) {
   if (IsXULElement(nsGkAtoms::label)) {
     nsAutoString control;
     GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
     if (control.IsEmpty()) {
-      return false;
+      return Err(NS_ERROR_UNEXPECTED);
     }
 
     // XXXsmaug Should we use ShadowRoot::GetElementById in case
     //          element is in Shadow DOM?
     RefPtr<Document> document = GetUncomposedDoc();
     if (!document) {
-      return false;
+      return Err(NS_ERROR_UNEXPECTED);
     }
 
     RefPtr<Element> element = document->GetElementById(control);
     if (!element) {
-      return false;
+      return Err(NS_ERROR_UNEXPECTED);
     }
 
     // XXXedgar, This is mainly for HTMLElement which doesn't do visible
@@ -488,7 +488,7 @@ bool nsXULElement::PerformAccesskey(bool aKeyCausesActivation,
     // label XULelement per spec.
     nsIFrame* frame = element->GetPrimaryFrame();
     if (!frame || !frame->IsVisibleConsideringAncestors()) {
-      return false;
+      return Err(NS_ERROR_UNEXPECTED);
     }
 
     return element->PerformAccesskey(aKeyCausesActivation, aIsTrustedEvent);
@@ -496,7 +496,7 @@ bool nsXULElement::PerformAccesskey(bool aKeyCausesActivation,
 
   nsIFrame* frame = GetPrimaryFrame();
   if (!frame || !frame->IsVisibleConsideringAncestors()) {
-    return false;
+    return Err(NS_ERROR_UNEXPECTED);
   }
 
   bool focused = false;
@@ -529,9 +529,13 @@ bool nsXULElement::PerformAccesskey(bool aKeyCausesActivation,
   if (aKeyCausesActivation && !IsXULElement(nsGkAtoms::menulist)) {
     ClickWithInputSource(MouseEvent_Binding::MOZ_SOURCE_KEYBOARD,
                          aIsTrustedEvent);
+    return focused;
   }
 
-  return focused;
+  // If the accesskey won't cause the activation and the focus isn't changed,
+  // either. Return error so EventStateManager would try to find next element
+  // to handle the accesskey.
+  return focused ? Result<bool, nsresult>{focused} : Err(NS_ERROR_ABORT);
 }
 
 //----------------------------------------------------------------------

dom/xul/nsXULElement.h

@@ -376,7 +376,8 @@ class nsXULElement : public nsStyledElement {
   MOZ_CAN_RUN_SCRIPT bool HasMenu();
   MOZ_CAN_RUN_SCRIPT void OpenMenu(bool aOpenFlag);
 
-  MOZ_CAN_RUN_SCRIPT virtual bool PerformAccesskey(
+  MOZ_CAN_RUN_SCRIPT
+  virtual mozilla::Result<bool, nsresult> PerformAccesskey(
       bool aKeyCausesActivation, bool aIsTrustedEvent) override;
   void ClickWithInputSource(uint16_t aInputSource, bool aIsTrustedEvent);