Support sequential focus navigation #15936

nox · 2017-03-14T09:14:35Z

This change is

highfive · 2017-03-14T09:14:38Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/script/dom/htmlanchorelement.rs, components/script/dom/element.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/htmlanchorelement.rs, components/script/dom/element.rs

highfive · 2017-03-14T09:14:39Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

nox · 2017-03-14T09:15:19Z

I've tested this locally, it works, but it cycles through every focusable element, not just form elements. Do we care?

nox · 2017-03-14T09:17:30Z

r? @jdm

I can't always be taking tasks from you Josh, sometimes I need to give some to you.

jdm

It's a good start, but there are a bunch of tricky details that we should sort out before merging this, I think.

jdm · 2017-03-20T15:00:09Z

components/script/dom/htmlanchorelement.rs

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


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.

jdm · 2017-03-20T15:01:40Z

components/script/dom/document.rs

+    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();


Unwrap looks scary here.

jdm · 2017-03-20T15:03:02Z

components/script/dom/document.rs

@@ -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 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.

jdm · 2017-03-20T15:03:43Z

components/script/dom/document.rs

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


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

jdm · 2017-03-20T15:06:20Z

components/script/dom/element.rs

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


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

jdm · 2017-03-20T15:09:19Z

components/script/dom/document.rs

@@ -1343,6 +1365,9 @@ impl Document {
        event.fire(target);
        let mut cancel_state = event.get_cancel_state();

+        self.handle_event(event);


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

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

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

Thanks! I'll try it!

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?

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.

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!

jdm · 2017-03-20T15:09:53Z

components/script/dom/document.rs

@@ -1343,6 +1365,9 @@ impl Document {
        event.fire(target);
        let mut cancel_state = event.get_cancel_state();

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


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

jdm · 2017-03-20T15:10:44Z

components/script/dom/element.rs

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


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.

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.

jdm · 2017-03-20T15:11:30Z

components/script/dom/element.rs

@@ -2260,10 +2274,14 @@ impl VirtualMethods for Element {
        }

        let doc = document_from_node(self);
+
+        doc.remove_focusable_element(self);


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

jdm · 2017-04-12T03:49:58Z

Closing since nobody is working on this.

CYBAI · 2018-01-01T08:43:21Z

components/script/dom/document.rs

+        // Step 1
+        // Step 2
+        // TODO: Implement the sequential focus navigation starting point.
+        // This can be used to change the starting point for navigation.


@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.

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

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

highfive assigned SimonSapin Mar 14, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 14, 2017

nox mentioned this pull request Mar 14, 2017

Support sequential focus navigation #13460

Closed

4 tasks

highfive assigned jdm and unassigned SimonSapin Mar 14, 2017

nox force-pushed the focus branch from f85b3c4 to 6577e27 Compare March 14, 2017 12:52

Support sequential focus navigation

c47e6f4

nox force-pushed the focus branch from 6577e27 to c47e6f4 Compare March 14, 2017 12:59

jdm requested changes Mar 20, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-new-owner The PR has been abandoned by the original author. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 20, 2017

jdm mentioned this pull request Mar 20, 2017

Implement form keyboard navigation #3982

Open

jdm closed this Apr 12, 2017

CYBAI reviewed Jan 1, 2018

View reviewed changes

CYBAI mentioned this pull request Feb 19, 2018

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sequential focus navigation #15936

Support sequential focus navigation #15936

nox commented Mar 14, 2017 •

edited by larsbergstrom

Loading

highfive commented Mar 14, 2017

highfive commented Mar 14, 2017

nox commented Mar 14, 2017

nox commented Mar 14, 2017

jdm left a comment

jdm Mar 20, 2017

jdm Mar 20, 2017

jdm Mar 20, 2017

jdm Mar 20, 2017

jdm Mar 20, 2017

jdm Mar 20, 2017

CYBAI Dec 23, 2017

jdm Dec 23, 2017

CYBAI Dec 24, 2017

CYBAI Dec 24, 2017

jdm Dec 24, 2017

CYBAI Dec 24, 2017 •

edited

Loading

jdm Mar 20, 2017

jdm Mar 20, 2017

CYBAI Jan 1, 2018

jdm Mar 20, 2017

jdm commented Apr 12, 2017

CYBAI Jan 1, 2018

jdm Jan 3, 2018

CYBAI Jan 4, 2018

Support sequential focus navigation #15936

Support sequential focus navigation #15936

Conversation

nox commented Mar 14, 2017 • edited by larsbergstrom Loading

highfive commented Mar 14, 2017

highfive commented Mar 14, 2017

nox commented Mar 14, 2017

nox commented Mar 14, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox commented Mar 14, 2017 •

edited by larsbergstrom

Loading

CYBAI Dec 24, 2017 •

edited

Loading