Skip to content

GitLab

T
tangled
Commit e33f399f authored by Jeonghee Ahn's avatar Jeonghee Ahn Committed by Commit Bot
Browse files
Options

[SpatNav] Fix crash of multiple select element on spatial navigation mode 


If no option has been selected and no option has been get focused by spatial navigation,
"active_selection_end_" has "null" value.

So, direct access to "active_selection_end_" seems unsafe.
The bug only reproduces if no prior selection has been made (=> active_selection_end_ has not been set).

In order to prevent this crash, I added validating of "active_selection_end_" and used "NextSelectableOption()"

The existing "snav-multiple-select.html" conduct test for option selection after focus moving test, so it does not cause crash.
So I added a TC for ensure the operation for selecting options.


Bug: 1006116
Change-Id: Iec277b52272ec1491749b14ec4720f3ff0e578c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1820924
Commit-Queue: Jeonghee Ahn <jeonghee27.ahn@lge.com>
Reviewed-by: default avatarFredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#700196}
parent b191f1b1
Hide whitespace changes
Inline Side-by-side
Showing with 108 additions and 6 deletions
third_party/blink/renderer/core/html/forms/html_select_element.cc
View file @ e33f399f
...@@ -1707,12 +1707,18 @@ void HTMLSelectElement::ListBoxDefaultEventHandler(Event& event) { ...@@ -1707,12 +1707,18 @@ void HTMLSelectElement::ListBoxDefaultEventHandler(Event& event) {
event.SetDefaultHandled(); event.SetDefaultHandled();
} else if (is_multiple_ && key_code == ' ' && } else if (is_multiple_ && key_code == ' ' &&
IsSpatialNavigationEnabled(GetDocument().GetFrame())) { IsSpatialNavigationEnabled(GetDocument().GetFrame())) {
// Use space to toggle selection change. HTMLOptionElement* option = active_selection_end_;
active_selection_state_ = !active_selection_state_; // If there's no active selection,
UpdateSelectedState(active_selection_end_.Get(), true /*multi*/, // act as if "ArrowDown" had been pressed.
false /*shift*/); if (!option)
ListBoxOnChange(); option = NextSelectableOption(LastSelectedOption());
event.SetDefaultHandled(); if (option) {
// Use space to toggle selection change.
active_selection_state_ = !active_selection_state_;
UpdateSelectedState(option, true /*multi*/, false /*shift*/);
ListBoxOnChange();
event.SetDefaultHandled();
}
} }
} }
} }
......
third_party/blink/web_tests/fast/spatial-navigation/snav-multiple-select-options.html 0 → 100644
View file @ e33f399f
<!DOCTYPE html>
<!--
This test ensures the operation of selecting on multiple select element on Spatial Navigation (SNav) mode.
* Pre-conditions:
1) DRT support for SNav enable/disable.
* Navigation steps:
1) Loads this page, focus goes to "start" automatically.
2) Some options are selected by Up/Down and space-bar key.
-->
<select id="start" multiple>
<option>1</option>
<option>2</option>
<option disabled>3</option>
<option>4</option>
</select>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="resources/snav-testharness.js"></script>
<script>
snav.assertSnavEnabledAndTestable();
test(() => {
const start = document.getElementById("start");
const options = start.options;
start.focus();
assert_false(options[0].selected, "options[0].selected should be false.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown(" "); //select 1st item
assert_true(options[0].selected, "options[0].selected should be true.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown(" "); //deselect 1st item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown("ArrowDown"); //move to 2nd item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown(" "); //select 2nd item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_true(options[1].selected, "options[1].selected should be true.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown("ArrowDown"); //move to 4th item (3rd item is disabled)
assert_false(options[0].selected, "options[0].selected should be false.");
assert_true(options[1].selected, "options[1].selected should be true.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_false(options[3].selected, "options[3].selected should be false.");
eventSender.keyDown(" "); //select 4th item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_true(options[1].selected, "options[1].selected should be true.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_true(options[3].selected, "options[3].selected should be true.");
eventSender.keyDown("ArrowUp"); //move back to 2nd item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_true(options[1].selected, "options[1].selected should be true.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_true(options[3].selected, "options[3].selected should be true.");
eventSender.keyDown(" "); //deselect 2nd item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_true(options[3].selected, "options[3].selected should be true.");
eventSender.keyDown("ArrowUp"); //move back to 1st item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_false(options[1].selected, "options[1].selected should be false.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_true(options[3].selected, "options[3].selected should be true.");
eventSender.keyDown("ArrowDown", ["shiftKey"]); //shift-down to 2nd item
assert_false(options[0].selected, "options[0].selected should be false.");
assert_true(options[1].selected, "options[1].selected should be true.");
assert_false(options[2].selected, "options[2].selected should be false.");
assert_true(options[3].selected, "options[3].selected should be true.");
});
</script>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment