Closed Bug 1327129 Opened 7 years ago Closed 7 years ago

Spacebar doesn't work in <select multiple> unless I made some non-trivial interaction with <select>

Categories

(Core :: Layout: Form Controls, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: MatsPalmgren_bugz)

Details

(Keywords: testcase)

Attachments

(2 files)

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url   data:text/html,<select multiple><option>1<option>2<option>3
2. Click on the page, press Tab key to focus <select>
3. Press Ctrl+Spacebar to select focused option in <select>

AR:
 Step 2 - the 1st option in select becomes focused (displays focusring).
 Step 3 - no visible action
ER:  Either X or Y.  X is better.
 X) Step 3 - focused option should be toggled (selected/not selected). See STR_2, STR_3.
 Y) Step 2 - 1st option shouldn't display focusring. For example, <select> should display focusring
             instead, and items should display it only if I select them via (Ctrl+)Up/Down/click.



STR_2:  (inconsistency with STR_1)
1. Open url   data:text/html,<select multiple><option>1<option>2<option>3
   Click on the page, press Tab key to focus <select>, press Ctrl+Down to "fix" the <select>
2. Click on the page, press Tab key to focus <select>
3. Press Ctrl+Spacebar to select focused option in <select>

AR:
 Step 2 - the 1st option in select becomes focused (displays focusring)
 Step 3 - the 1st option in select becomes selected, just as expected
         (compare this to STR_1: user sees the same thing in Steps 2,3, but behaves differently)



STR_3:  (reference of good behavior)
1. Open bookmarks sidebar (Ctrl+B)
2. Press Tab key to focus the 1st bookmark
3. Press Ctrl+Spacebar to select focused bookmark in <tree>

AR:  Focused bookmark becomes selected, just as expected
No longer blocks: 1277113
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Assignee: nobody → mats
Severity: normal → minor
Has STR: --- → yes
Keywords: testcase
OS: Unspecified → All
Priority: -- → P4
Hardware: Unspecified → All
BTW, I tested Chrome (Linux) and Edge (Windows) manually on the various <select>s
in the test, and it seems they have similar behavior [of acting on the first option]
in most cases.
Comment on attachment 8823865 [details] [diff] [review]
Make key events act on the first non-disabled <option> (if any) when no <option> is selected (i.e. selectedIndex=-1).

>-  for (uint32_t i = 0, length = selectElement->Length(); i < length; ++i) {
>-    dom::HTMLOptionElement* node = selectElement->Item(i);
>+  uint32_t i = std::max(aFromIndex, 0);
>+  for (const uint32_t length = selectElement->Length(); i < length; ++i) {
Tiny bit unusual to fine i outside for() but length inside. but fine.


>+    HTMLOptionElement* node = selectElement->Item(i);
>     if (!node) {
>-      return nullptr;
>+      break;
>     }
>-
>     if (!selectElement->IsOptionDisabled(node)) {
>+      if (aFoundIndex) {
>+        *aFoundIndex = i;
>+      }
>       return node;
>     }
>   }
>-
>   return nullptr;
> }
> 
> bool
> nsListControlFrame::IsInDropDownMode() const
> {
>   return (mComboboxFrame != nullptr);
> }
>@@ -2474,17 +2483,27 @@ nsListControlFrame::KeyPress(nsIDOMEvent
> 
> void
> nsListControlFrame::PostHandleKeyEvent(int32_t aNewIndex,
>                                        uint32_t aCharCode,
>                                        bool aIsShift,
>                                        bool aIsControlOrMeta)
> {
>   if (aNewIndex == kNothingSelected) {
>-    return;
>+    int32_t focusedIndex = mEndSelectionIndex == kNothingSelected ?
>+      GetSelectedIndex() : mEndSelectionIndex;
>+    if (focusedIndex != kNothingSelected) {
>+      return;
>+    }
>+    // No options are selected.  In this case the focus ring is on the first
>+    // non-disabled option (if any), so we should behave as if that's the option
>+    // the user acted on.
>+    if (!GetNonDisabledOptionFrom(0, &aNewIndex)) {
>+      return;
>+    }
>   }
> 
>   // If you hold control, but not shift, no key will actually do anything
>   // except space.
>   nsWeakFrame weakFrame(this);
>   bool wasChanged = false;
>   if (aIsControlOrMeta && !aIsShift && aCharCode != ' ') {
>     mStartSelectionIndex = aNewIndex;
>diff --git a/layout/forms/nsListControlFrame.h b/layout/forms/nsListControlFrame.h
>--- a/layout/forms/nsListControlFrame.h
>+++ b/layout/forms/nsListControlFrame.h
>@@ -44,16 +44,18 @@ class HTMLOptionsCollection;
>  */
> 
> class nsListControlFrame final : public nsHTMLScrollFrame,
>                                  public nsIFormControlFrame,
>                                  public nsIListControlFrame,
>                                  public nsISelectControlFrame
> {
> public:
>+  typedef mozilla::dom::HTMLOptionElement HTMLOptionElement;
>+
>   friend nsContainerFrame* NS_NewListControlFrame(nsIPresShell* aPresShell,
>                                                   nsStyleContext* aContext);
> 
>   NS_DECL_QUERYFRAME
>   NS_DECL_FRAMEARENA_HELPERS
> 
>     // nsIFrame
>   virtual nsresult HandleEvent(nsPresContext* aPresContext,
>@@ -113,17 +115,17 @@ public:
>     // for accessibility purposes
> #ifdef ACCESSIBILITY
>   virtual mozilla::a11y::AccType AccessibleType() override;
> #endif
> 
>     // nsIListControlFrame
>   virtual void SetComboboxFrame(nsIFrame* aComboboxFrame) override;
>   virtual int32_t GetSelectedIndex() override;
>-  virtual mozilla::dom::HTMLOptionElement* GetCurrentOption() override;
>+  virtual HTMLOptionElement* GetCurrentOption() override;
> 
>   /**
>    * Gets the text of the currently selected item.
>    * If the there are zero items then an empty string is returned
>    * If there is nothing selected, then the 0th item's text is returned.
>    */
>   virtual void GetOptionText(uint32_t aIndex, nsAString& aStr) override;
> 
>@@ -175,17 +177,17 @@ public:
> 
>   /**
>    * Returns the options collection for mContent, if any.
>    */
>   mozilla::dom::HTMLOptionsCollection* GetOptions() const;
>   /**
>    * Returns the HTMLOptionElement for a given index in mContent's collection.
>    */
>-  mozilla::dom::HTMLOptionElement* GetOption(uint32_t aIndex) const;
>+  HTMLOptionElement* GetOption(uint32_t aIndex) const;
> 
>   static void ComboboxFocusSet();
> 
>   // Helper
>   bool IsFocused() { return this == mFocused; }
> 
>   /**
>    * Function to paint the focus rect when our nsSelectsAreaFrame is painting.
>@@ -253,16 +255,23 @@ public:
>    * fire a native focus event for accessibility
>    * (Some 3rd party products need to track our focus)
>    */
>   void FireMenuItemActiveEvent(); // Inform assistive tech what got focused
> #endif
> 
> protected:
>   /**
>+   * Return the first non-disabled option starting at aFromIndex (inclusive).
>+   * @param aFoundIndex if non-null, set to the index of the returned option 
>+   */
>+  HTMLOptionElement* GetNonDisabledOptionFrom(int32_t aFromIndex,
>+                                              int32_t* aFoundIndex = nullptr);
>+
>+  /**
>    * Updates the selected text in a combobox and then calls FireOnChange().
>    * @note This method might destroy the frame, pres shell and other objects.
>    * Returns false if calling it destroyed |this|.
>    */
>   bool       UpdateSelection();
> 
>   /**
>    * Returns whether mContent supports multiple selection.
>@@ -277,17 +286,17 @@ protected:
>    * @note This method might destroy the frame, pres shell and other objects.
>    */
>   void       DropDownToggleKey(nsIDOMEvent* aKeyEvent);
> 
>   nsresult   IsOptionDisabled(int32_t anIndex, bool &aIsDisabled);
>   /**
>    * @note This method might destroy the frame, pres shell and other objects.
>    */
>-  void ScrollToFrame(mozilla::dom::HTMLOptionElement& aOptElement);
>+  void ScrollToFrame(HTMLOptionElement& aOptElement);
>   /**
>    * @note This method might destroy the frame, pres shell and other objects.
>    */
>   void ScrollToIndex(int32_t anIndex);
> 
>   /**
>    * When the user clicks on the comboboxframe to show the dropdown
>    * listbox, they then have to move the mouse into the list. We don't
Attachment #8823865 - Flags: review?(bugs) → review+
> Tiny bit unusual to fine i outside for() but length inside. but fine.

Good point, I'll swap them.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6c4863ab27
Make key events act on the first non-disabled <option> (if any) when no <option> is selected (i.e. selectedIndex=-1).  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa1f8402f8c
Test key events on various <select> elements with selectedIndex=-1.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/fb6c4863ab27
https://hg.mozilla.org/mozilla-central/rev/5aa1f8402f8c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: