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)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: arni2033, Assigned: MatsPalmgren_bugz)
Details
(Keywords: testcase)
Attachments
(2 files)
6.54 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
11.80 KB,
patch
|
Details | Diff | Splinter Review |
>>> 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
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mats
Severity: normal → minor
Has STR: --- → yes
Keywords: testcase
OS: Unspecified → All
Priority: -- → P4
Hardware: Unspecified → All
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8823865 -
Flags: review?(bugs)
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60fc8f6b3c50a0c58ed1bae6389b13f5ad1d12a2
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
> 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.
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite+
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb6c4863ab27 https://hg.mozilla.org/mozilla-central/rev/5aa1f8402f8c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•