fixes #5603, added support for tabindex field

[pgonda]

Added support for the tabindex field, also added its correct defaults (-2 TODOs for things not supported in Servo yet). Also added tabindex logic into Element::is_focusable_area.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/4808

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[pgonda]

updated and squashed, r+?

[jdm]

I'm partway through reviewing this.

[bors-servo]

☔ The latest upstream changes (presumably #5888) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

Ok, this generally looks good, but there are a few subtleties that I don't think we're dealing with. The flag should be updated whenever the value of tabindex or draggable is updated/removed (we'll need a before_remove_attr method too, as mentioned in one of my comments). I think we should extract all of the checks from after_set_attr/bind_to_tree into a helper method named udpate_sequentially_focusable_status, which gets called from after_set_attr/before_remove_attr and takes arguments for the presence/values of tabindex/draggable. Does that make sense?

-S-awaiting-review +S-needs-code-changes

---

Reviewed files:

• components/script/dom/element.rs @ r3, r5
• components/script/dom/htmlelement.rs @ r5
• components/script/dom/node.rs @ r5

---

^{components/script/dom/htmlelement.rs, line 239 [r3] (raw file):}
This should be an exact match.

---

^{components/script/dom/htmlelement.rs, line 240 [r3] (raw file):}
No need for : JSRef<Node> any more.

---

^{components/script/dom/htmlelement.rs, line 241 [r3] (raw file):}
There should be a corresponding before_remove_attr method that clears this flag if necessary, too.

---

^{components/script/dom/htmlelement.rs, line 248 [r3] (raw file):}
No need for : &JSRef<Element> any more.

---

^{components/script/dom/htmlelement.rs, line 249 [r3] (raw file):}
Let's use has_attribute instead.

---

^{components/script/dom/htmlelement.rs, line 250 [r3] (raw file):}
No need for the explicit type any more.

---

^{components/script/dom/htmlelement.rs, line 258 [r3] (raw file):}
has_attribute

---

^{components/script/dom/htmlelement.rs, line 263 [r3] (raw file):}
if let

---

^{components/script/dom/htmlelement.rs, line 267 [r3] (raw file):}
value == "true"

---

^{components/script/dom/node.rs, line 162 [r5] (raw file):}
Let's call this SEQUENTIALLY_FOCUSABLE instead.

---

---

Comments from the review on Reviewable.io

[pgonda]

@jdm How does that look? I extracted all the logic out into one method that works for both default values if no tabindex flag is present or if a flag is added/removed

[pgonda]

@jdm r?

[jdm]

Sorry, I've started reviewing this several times and realized that I didn't have enough time to think carefully about how all the parts fit together. I'll have a review finished before Wednesday.

[pgonda]

No problem! Take your time no worries.

[jdm]

Sorry about the delay!
-S-awaiting-review +S-needs-code-changes +S-needs-rebase

---

Reviewed files:

• components/script/dom/element.rs @ r6
• components/script/dom/htmlelement.rs @ r6
• components/script/dom/node.rs @ r6

---

^{components/script/dom/htmlelement.rs, line 82 [r6] (raw file):}
self, not &self. I also don't think we need to pass any arguments here - we can rely entirely on "does this attribute exist" and "what value does this attribute" have checks, and initialize SEQUENTIALLY_FOCUSED to false each time.

---

^{components/script/dom/htmlelement.rs, line 83 [r6] (raw file):}
from_ref, and no need for the : JSRef<Element> anymore.

---

^{components/script/dom/htmlelement.rs, line 95 [r6] (raw file):}
nit: unindent by four spaces.

---

^{components/script/dom/htmlelement.rs, line 99 [r6] (raw file):}
nit: unindent by four spaces

---

^{components/script/dom/htmlelement.rs, line 113 [r6] (raw file):}
let is_true = value.as_slice() == "true";
node.set_flat(SEQUENTIALLY_FOCUSABLE, is_true);

---

^{components/script/dom/htmlelement.rs, line 266 [r6] (raw file):}
I was wrong about this; since we don't actually care about the value of the attribute, we really should use the after_remove_attr hook instead. HOWEVER that doesn't exist! You should add it in dom/virtualmethods.rs and use it here; it should take an &Atom argument which is the name of the attribute that was removed.

---

Comments from the review on Reviewable.io

[pgonda]

I added the new virtual method after_remove_attr do In VirtualMethods.rs do I need to implement/class this somewhere else other than htmlelement.rs?

[jdm]

I don't think so.

[jdm]

Oh, I wasn't thinking - this will need to be called (just like before_remove_attr) from element.rs.

-S-awaiting-review -S-needs-rebase +S-needs-code-changes

---

Reviewed files:

• components/script/dom/element.rs @ r7
• components/script/dom/htmlelement.rs @ r7
• components/script/dom/node.rs @ r7
• components/script/dom/virtualmethods.rs @ r7

---

Comments from the review on Reviewable.io

[bors-servo]

💔 Test failed - mac1

[Ms2ger]

/html/semantics/disabled-elements/disabledElement.html
------------------------------------------------------
PASS expected FAIL A disabled <span> should be focusable
/html/semantics/selectors/pseudo-classes/focus.html
---------------------------------------------------
PASS expected FAIL tabindex attribute makes the element focusable
PASS expected FAIL ':focus' matches focussed body with tabindex

[pgonda]

This is supposed to PASS now, how can I change that?

[jdm]

Modify the contents of http://mxr.mozilla.org/servo/source/tests/wpt/metadata/html/semantics/disabled-elements/disabledElement.html.ini and http://mxr.mozilla.org/servo/source/tests/wpt/metadata/html/semantics/selectors/pseudo-classes/focus.html.ini (and remove any "expected: PASS" statements, or the whole file if there are no remaining failures).

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 1c96eed has been approved by jdm

[bors-servo]

⌛ Testing commit 1c96eed with merge fada391...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2