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

[WIP] script: Partially implement sequential focus navigation #20075

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

script: Partially implement sequential focus navigation

  • Loading branch information
CYBAI committed Jan 29, 2019
commit 836a34d4f8237bff8f063d9926baac8ec4bb928d
@@ -359,6 +359,8 @@ pub struct Document {
load_event_end: Cell<u64>,
unload_event_start: Cell<u64>,
unload_event_end: Cell<u64>,
/// Current focus node element
focus_starting_point: MutNullableDom<Element>,
/// <https://html.spec.whatwg.org/multipage/#concept-document-https-state>
https_state: Cell<HttpsState>,
/// The document's origin.
@@ -746,6 +748,20 @@ impl Document {
self.reset_form_owner_for_listeners(&id);
}

pub fn reset_focus_starting_point(&self) {
self.focus_starting_point.set(None);
}

/// Set focus starting point
pub fn set_focus_starting_point(&self, element: &Element) {
self.focus_starting_point.set(Some(element));
}

/// Get focus starting point
pub fn get_focus_starting_point(&self) -> Option<DomRoot<Element>> {
self.focus_starting_point.get()
}

/// Associate an element present in this document with the provided id.
pub fn register_named_element(&self, element: &Element, id: Atom) {
debug!(
@@ -1412,6 +1428,15 @@ impl Document {
event.fire(target);
let mut cancel_state = event.get_cancel_state();

if let Some(ref key_event) = event.downcast::<KeyboardEvent>() {

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

I think we should detect the tab key in the embedding layer, queue a WindowEvent for advancing the sequential focus that is processed here, which will send a special event to the script thread to perform the sequential focus change, rather than detecting keyboard events in the script thread.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 30, 2019

Author Collaborator

Thanks for pointing me to the embedding layer! It must be the right place to detect the tab key :D Will update!

if event.type_() == atom!("keydown") {
if let Key::Tab = key_event.key() {
self.sequential_focus_navigation(key_event);
self.commit_focus_transaction(FocusType::Element);
}
}
}

// https://w3c.github.io/uievents/#keys-cancelable-keys
if keyboard_event.state == KeyState::Down &&
keyboard_event.key.legacy_charcode() != 0 &&
@@ -2416,6 +2441,135 @@ 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
let starting_point = match self.get_currently_focused_area_from_top_level() {
Some(point) => {
match self.get_focus_starting_point() {
Some(focused) => {
// Step 2
if focused
.upcast::<Node>()
.is_ancestor_of(self.upcast::<Node>())
{
Some(focused)
} else {
Some(point)
}
},
_ => Some(point),
}
},
None => None,
};

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

// TODO(cybai): Step 4
let selection_mechanism = SelectionMechanism::Sequential;

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

// Step 6, 7
if let Some(ref element) = candidate {
self.request_focus(element);
}

// TODO(cybai): Step 8, 9
}

/// https://html.spec.whatwg.org/multipage/#sequential-navigation-search-algorithm
fn sequential_search(
&self,
starting_point: Option<DomRoot<Element>>,
direction: NavigationDirection,
// TODO(cybai): Use selection mechanism for searching
_selection_mechanism: SelectionMechanism,
) -> Option<DomRoot<Element>> {
// Step 1-1
let next_focusable_element = self.find_next_focusable_element(starting_point, direction);

match next_focusable_element {
Some(element) => {
self.set_focus_starting_point(&element);
Some(element)
},
None => self.GetDocumentElement(),
}

// TODO(cybai): Step 1-2, 1-3, 2, 3
}

fn find_next_focusable_element(
&self,
starting_point: Option<DomRoot<Element>>,
direction: NavigationDirection,
) -> Option<DomRoot<Element>> {
let mut elements = self
.upcast::<Node>()
.traverse_preorder()
.filter_map(DomRoot::downcast::<Element>)
.filter(|element| element.is_focusable_area() && !element.disabled_state());

// Use a `prev_element` to memoize the previous element,
// in `forward` direction, we'll compare if the previous element
// is `starting point`, if yes, return current element
// in `backward` direction, we'll compare if the current element
// is `starting point`, if yes, return previous element
let first_element = elements.nth(0);
let mut prev_element: Option<DomRoot<Element>> = None;
for element in elements {
if let Some(ref point) = starting_point {

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

I think it would be more clear to do this:

let starting_point = match starting_point {
    Some(ref point) => point,
    None => return first_element,
};
for element in elements {
    ...
}
if let Some(prev) = prev_element {
if prev == *point && direction == NavigationDirection::Forward {
return Some(element);
}

if element == *point && direction == NavigationDirection::Backward {
return Some(prev);
}
}
}
prev_element = Some(element);
}

first_element
}

/// https://html.spec.whatwg.org/multipage/#currently-focused-area-of-a-top-level-browsing-context
fn get_currently_focused_area_from_top_level(&self) -> Option<DomRoot<Element>> {
if !self.window.is_top_level() {

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

It looks like we should get this document's browsing context, then call top() to get the top-level window, then get the document from that, rather than returning immediately here.

return None;
}

// TODO: Step 2-1

// Step 2-2, 2-3
// TODO: Need to loop 2-2 to find active document
match self.get_focused_element() {
Some(focused) => Some(focused),
None => {
if self.get_focus_starting_point().is_some() {
return self.get_focus_starting_point();
}

if self.has_browsing_context {
return self.GetDocumentElement();

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

Which part of the spec is this implementing?

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 30, 2019

Author Collaborator

hmm... I can't recall which part of the spec I refer to... maybe I need to re-read the spec and check the implementation again 😓

}

None
},
}
}

/// <https://html.spec.whatwg.org/multipage/#look-up-a-custom-element-definition>
pub fn lookup_custom_element_definition(
&self,
@@ -2721,6 +2875,7 @@ impl Document {
load_event_end: Cell::new(Default::default()),
unload_event_start: Cell::new(Default::default()),
unload_event_end: Cell::new(Default::default()),
focus_starting_point: MutNullableDom::new(None),
https_state: Cell::new(HttpsState::None),
origin: origin,
referrer: referrer,
@@ -3273,6 +3428,22 @@ impl Document {
}
}

/// Specifies the type of sequential focus direction
#[derive(Eq, PartialEq)]
enum NavigationDirection {
Forward,
Backward,
}

/// Specifies the type of selection mechanism for sequential focus
enum SelectionMechanism {
// TODO(cybai): Temporarily allow dead code here. It should be removed
// after implementing selection mechanism for sequential focus search
#[allow(dead_code)]
DOM,
Sequential,
}

impl Element {
fn click_event_filter_by_disabled_state(&self) -> bool {
let node = self.upcast::<Node>();
@@ -2716,6 +2716,14 @@ impl VirtualMethods for Element {
}

let doc = document_from_node(self);

if self.is_focusable_area() &&
!self.disabled_state() &&
doc.get_focus_starting_point().is_none()

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

What if we move this logic into set_focus_starting_point? It feels like the document should be responsible for deciding whether to actually update its member.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Jan 30, 2019

Author Collaborator

That sounds reasonable to me! Will update!

{
doc.set_focus_starting_point(&self);
}

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

let doc = document_from_node(self);

// TODO: (cybai) Reset focus starting point if current element is current focus starting point
// . and it's removed from the tree.

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

This feels like we should call document.focusable_element_removed(self) and the document can decide what to do with that information.


let fullscreen = doc.GetFullscreenElement();
if fullscreen.r() == Some(self) {
doc.exit_fullscreen();
@@ -20,12 +20,14 @@ use crate::dom::event::Event;
use crate::dom::eventtarget::EventTarget;
use crate::dom::htmlelement::HTMLElement;
use crate::dom::htmlimageelement::HTMLImageElement;
use crate::dom::keyboardevent::KeyboardEvent;
use crate::dom::mouseevent::MouseEvent;
use crate::dom::node::{document_from_node, Node};
use crate::dom::urlhelper::UrlHelper;
use crate::dom::virtualmethods::VirtualMethods;
use dom_struct::dom_struct;
use html5ever::{LocalName, Prefix};
use keyboard_types::Key;
use net_traits::ReferrerPolicy;
use num_traits::ToPrimitive;
use servo_url::ServoUrl;
@@ -111,6 +113,20 @@ impl VirtualMethods for HTMLAnchorElement {
.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>() {

This comment has been minimized.

Copy link
@jdm

jdm Jan 30, 2019

Member

I have filed #22782 about dealing with this properly.

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

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.