Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support sequential focus navigation #13460

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Process `Tab` and `Shift+Tab` key events

This causes the document focus to transition to the next/previous
element in the sequential focus ordering list.
  • Loading branch information
nerith committed Sep 27, 2016
commit c143ec0f2fe8510bdef2635fcf7ebf600a84c21c
@@ -83,6 +83,7 @@ use dom::touchevent::TouchEvent;
use dom::touchlist::TouchList;
use dom::treewalker::TreeWalker;
use dom::uievent::UIEvent;
use dom::virtualmethods::VirtualMethods;
use dom::webglcontextevent::WebGLContextEvent;
use dom::window::{ReflowReason, Window};
use encoding::EncodingRef;
@@ -240,6 +241,7 @@ pub struct Document {
load_event_end: Cell<u64>,
/// Vector to store focusable elements
focus_list: DOMRefCell<Vec<JS<Element>>>,
focus_list_index: Cell<u64>,

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

Why u64 and not usize?

/// https://html.spec.whatwg.org/multipage/#concept-document-https-state
https_state: Cell<HttpsState>,
touchpad_pressure_phase: Cell<TouchpadPressurePhase>,
@@ -1183,6 +1185,9 @@ impl Document {
event.fire(target);
let mut prevented = event.DefaultPrevented();

self.handle_event(event);
self.commit_focus_transaction(FocusType::Element);

// https://w3c.github.io/uievents/#keys-cancelable-keys
if state != KeyState::Released && props.is_printable() && !prevented {
// https://w3c.github.io/uievents/#keypress-event-order
@@ -1663,6 +1668,64 @@ impl Document {

self.window.layout().nodes_from_point(page_point, *client_point)
}

// https://html.spec.whatwg.org/multipage/#sequential-focus-navigation
fn sequential_focus_navigation(&self, event: &Event, key: Key) {
// Step 1
// Step 2
// TODO: Implement the sequential focus navigation starting point.
// This can be used to change the starting point for navigation.

// Step 3
let direction =
if key == Key::Tab {
let modifier = event.downcast::<KeyboardEvent>()

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

Could this method take a KeyboardEvent directly?

.unwrap().get_key_modifiers();

if modifier.is_empty() {
NavigationDirection::Forward
} else {
NavigationDirection::Backward
}
} else {

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

Why do we return if key isn't Key::Tab? Shouldn't instead this method be called only if that condition is true and the whole method not take a key argument at all?

return
};

// TODO: Step 4

// Step 5
let candidate = self.sequential_search(direction);

// Step 6
self.request_focus(&self.focus_list.borrow()[candidate]);
}

fn sequential_search(&self, direction: NavigationDirection) -> usize {

This comment has been minimized.

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

This is supposed to return a candidate; shouldn't the return type be Root<Element>?

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

To which behaviour this method correspond in the spec for that algorithm?

let list = self.focus_list.borrow();
let ref current_index = self.focus_list_index;
let focus_state = list[current_index.get() as usize].focus_state();

if focus_state {
match direction {
NavigationDirection::Forward => {
current_index.set(current_index.get() + 1);

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

I would rather not rely on overflow here. Could you explicitly check that current_index wasn't list.len() here?


if current_index.get() == (list.len() as u64) {
current_index.set(0);
}
},
NavigationDirection::Backward => {
current_index.set(current_index.get() - 1);

if current_index.get() >= (list.len() as u64) {
current_index.set((list.len() as u64) - 1);
}

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

I would rather not rely on underflow here. Could you explicitly check that current_index wasn't 0?

},
}
}

current_index.get() as usize
}
}

#[derive(PartialEq, HeapSizeOf)]
@@ -1807,6 +1870,7 @@ impl Document {
load_event_start: Cell::new(Default::default()),
load_event_end: Cell::new(Default::default()),
focus_list: DOMRefCell::new(vec![]),
focus_list_index: Cell::new(0),
https_state: Cell::new(HttpsState::None),
touchpad_pressure_phase: Cell::new(TouchpadPressurePhase::BeforeClick),
origin: origin,
@@ -1983,6 +2047,30 @@ impl Document {
}
}

impl VirtualMethods for Document {
fn super_type(&self) -> Option<&VirtualMethods> {
Some(self.upcast::<Document>() as &VirtualMethods)
}

fn handle_event(&self, event: &Event) {
if event.type_() == atom!("keydown") {
let key_event = event.downcast::<KeyboardEvent>().unwrap();

This comment has been minimized.

Copy link
@nox

nox Oct 17, 2016

Member

Try to downcast to KeyboardEvent, and only then check that its type is keydown.

let key = key_event.get_key().unwrap();

match key {
Key::Tab => {
self.sequential_focus_navigation(event, Key::Tab);
},
_ => (),
}
}
}
}

enum NavigationDirection {
Forward,
Backward
}

impl Element {
fn click_event_filter_by_disabled_state(&self) -> bool {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.