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 #15936

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

Always

Just for now

@@ -85,6 +85,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 dom_struct::dom_struct;
@@ -284,6 +285,9 @@ pub struct Document {
dom_complete: Cell<u64>,
load_event_start: Cell<u64>,
load_event_end: Cell<u64>,
/// Vector to store focusable elements
focus_list: DOMRefCell<Vec<JS<Element>>>,
focus_list_index: Cell<usize>,
/// https://html.spec.whatwg.org/multipage/#concept-document-https-state
https_state: Cell<HttpsState>,
touchpad_pressure_phase: Cell<TouchpadPressurePhase>,
@@ -592,6 +596,24 @@ impl Document {
}
}

/// Add a focusable element to this document's ordering focus list.
pub fn add_focusable_element(&self, element: &Element) {
self.focus_list.borrow_mut().push(JS::from_ref(element));

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

This does not account for the tree ordering of the elements, such as if a control is inserted dynamically into the tree after the initial page is parsed. Also, it doesn't protect against duplicates.

}

/// Removes a focusable element from this document's ordering focus list.
pub fn remove_focusable_element(&self, element: &Element) {
let mut list = self.focus_list.borrow_mut();

let index = list.iter().position(|item| {
&**item == element
});

if let Some(index) = index {
list.remove(index);
}

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

We will need to ensure that the current focused index is updated to account for this change.

}

/// Associate an element present in this document with the provided id.
pub fn register_named_element(&self, element: &Element, id: Atom) {
debug!("Adding named element to document {:p}: {:p} id={}",
@@ -1343,6 +1365,9 @@ impl Document {
event.fire(target);
let mut cancel_state = event.get_cancel_state();

self.handle_event(event);

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

I think we should inline the code from handle_event in here instead. No need to call a different method.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Dec 23, 2017

Collaborator

@jdm If we move the code from handle_event here, I think we don't need impl VirtualMethods for Document?

This comment has been minimized.

Copy link
@jdm

jdm Dec 23, 2017

Member

Go ahead and remove it if that's the case.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Dec 24, 2017

Collaborator

Thanks! I'll try it!

This comment has been minimized.

Copy link
@CYBAI

CYBAI Dec 24, 2017

Collaborator

According to this comment in the most original PR from @nox , I think that's why nox made it inside VirtualMethods.
However, after trying to make the code in here, the focus sequential still works.
Not sure if there's any concerns about moving the code in here instead of inside VirtualMethods?

This comment has been minimized.

Copy link
@jdm

jdm Dec 24, 2017

Member

The comment you point out was made in a version that did not include the parent type's handle_event call, which is required when using VirtualMethods. I don't think we need to use VirtualMethods for that code, however.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Dec 24, 2017

Collaborator

Understood. We can handle the event without VirtualMethods but I didn't know too much about it; thus, I left the comment.
Now, I think it should be okay to remove it :)
Thanks!

self.commit_focus_transaction(FocusType::Element);

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

Running this for every single key event that is processed seems odd.


// https://w3c.github.io/uievents/#keys-cancelable-keys
if state != KeyState::Released && props.is_printable() && cancel_state != EventDefault::Prevented {
// https://w3c.github.io/uievents/#keypress-event-order
@@ -1935,6 +1960,61 @@ impl Document {

self.window.layout().nodes_from_point_response()
}

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

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 1, 2018

Collaborator

@jdm I'm stuck by the add_focusable_element and remove_focusable_element methods above.

According to the spec,

When the user requests that focus move from the currently focused area of a top-level browsing context to the next or previous focusable area (e.g. as the default action of pressing the tab key), or when the user requests that focus sequentially move to a top-level browsing context in the first place (e.g. from the browser's location bar), the user agent must use the following algorithm

Thus, maybe we don't need to save a focus_list inside Document? We might only need to know the starting point?

I'm wondering if it's possible to calculate the starting point and also its next and previous focus candidate.

This comment has been minimized.

Copy link
@jdm

jdm Jan 3, 2018

Member

If we store the node for the starting point and not the list and starting index then it might be easier, like you suggest. It would require:

  • determining and storing the new starting point if the current starting point ever becomes unfocusable
  • recalculating the list every time we need to change the current starting point

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 4, 2018

Collaborator

@jdm Thanks for your confirmation. I'll try to only store the starting point for it!


let modifier = event.get_key_modifiers();

// Step 3
let direction =
if modifier.is_empty() {
NavigationDirection::Forward
} else {
NavigationDirection::Backward
};

// TODO: Step 4

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

// Step 6
self.request_focus(&candidate);
}

// https://html.spec.whatwg.org/multipage/#sequential-navigation-search-algorithm
// TODO: Use the starting point and selection mechanism for searching
fn sequential_search(&self, direction: NavigationDirection) -> Root<Element> {
let list = self.focus_list.borrow();
let ref current_index = self.focus_list_index;
let focus_state = list[current_index.get()].focus_state();

if focus_state {
match direction {
NavigationDirection::Forward => {
if current_index.get() != list.len() - 1 {
current_index.set(current_index.get() + 1);
} else {
current_index.set(0);
}
},
NavigationDirection::Backward => {
if current_index.get() != 0 {
current_index.set(current_index.get() - 1);
} else {
current_index.set(list.len() - 1);
}
},
}
}

Root::from_ref(&*list[current_index.get()])
}
}

#[derive(PartialEq, HeapSizeOf)]
@@ -2090,6 +2170,8 @@ impl Document {
dom_complete: Cell::new(Default::default()),
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,
@@ -2416,6 +2498,31 @@ 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 let Some(key_event) = event.downcast::<KeyboardEvent>() {
if event.type_() == atom!("keydown") {
let key = key_event.get_key().unwrap();

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

Unwrap looks scary here.


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

enum NavigationDirection {
Forward,
Backward
}

impl Element {
fn click_event_filter_by_disabled_state(&self) -> bool {
@@ -2245,6 +2245,20 @@ impl VirtualMethods for Element {
}

let doc = document_from_node(self);

if self.is_focusable_area() {
let doc = document_from_node(self);
let children = self.upcast::<Node>().traverse_preorder();

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

I don't think we want to iterate over all the children here, since they should have their own bind_to_tree methods called already.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 1, 2018

Collaborator

If we only try to find the starting point of focusable elements, I'll expect we don't need to traverse the children here and add them into the list.


for child in children {
if let Some(element) = child.downcast::<Element>() {
if element.is_focusable_area() && !element.disabled_state() {

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

We will need to account for elements that have their disabled state modified dynamically, too.

doc.add_focusable_element(element);
}
}
}
}

if let Some(ref value) = *self.id_attribute.borrow() {
doc.register_named_element(self, value.clone());
}
@@ -2260,10 +2274,14 @@ impl VirtualMethods for Element {
}

let doc = document_from_node(self);

doc.remove_focusable_element(self);

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

We should only call this if we know that this element is has previously been added to the list.


let fullscreen = doc.GetFullscreenElement();
if fullscreen.r() == Some(self) {
doc.exit_fullscreen();
}

if let Some(ref value) = *self.id_attribute.borrow() {
doc.unregister_named_element(self, value.clone());
}
@@ -20,12 +20,14 @@ use dom::event::Event;
use dom::eventtarget::EventTarget;
use dom::htmlelement::HTMLElement;
use dom::htmlimageelement::HTMLImageElement;
use dom::keyboardevent::KeyboardEvent;
use dom::mouseevent::MouseEvent;
use dom::node::{Node, document_from_node};
use dom::urlhelper::UrlHelper;
use dom::virtualmethods::VirtualMethods;
use dom_struct::dom_struct;
use html5ever_atoms::LocalName;
use msg::constellation_msg::Key;
use net_traits::ReferrerPolicy;
use num_traits::ToPrimitive;
use script_traits::MozBrowserEvent;
@@ -100,6 +102,23 @@ impl VirtualMethods for HTMLAnchorElement {
_ => self.super_type().unwrap().parse_plain_attribute(name, value),
}
}

fn handle_event(&self, event: &Event) {
if let Some(s) = self.super_type() {
s.handle_event(event);
}

if let Some(key_event) = event.downcast::<KeyboardEvent>() {
if event.type_() == atom!("keydown") {
match key_event.get_key() {
Some(Key::Enter) => {
follow_hyperlink(self.upcast::<Element>(), Some(String::new()), None);

This comment has been minimized.

Copy link
@jdm

jdm Mar 20, 2017

Member

We should be calling into activation_behaviour here, which will need to be updated to account for non-mouse activations. We should also handle space key events similarly.

},
_ => ()
}
}
}
}
}

impl HTMLAnchorElementMethods for HTMLAnchorElement {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.