1514570 - Only graphics elements should support tabindex

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Comment 1

[Robert Longson [:longsonr]]

Attachment: Part 1 - Move code to SVGGraphicsElement but special case <defs>

Gets us closer to the other UAs.

https://www.w3.org/TR/SVG2/interact.html#Focus

In Part 2 I'll implement support for focusable.

Comment 2

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a4c5cc0575a510513ada7cef7af6bdd3565f9a4

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 9031765 [details] [diff] [review]
Part 1 - Move code to SVGGraphicsElement but special case <defs>

Review of attachment 9031765 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, two nits:

::: dom/svg/SVGDefsElement.h
@@ +28,5 @@
>    // nsIContent
>    NS_IMETHOD_(bool) IsAttributeMapped(const nsAtom* aAttribute) const override;
>  
> +  bool IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse) override {
> +    return nsIContent::IsFocusableInternal(aTabIndex, aWithMouse);

Right now it's unclear that this method's implementation is really just a strict "no I'm not focusable".  Could you add a comment to make that clearer?

Also: it seems bizarre that SVGDefsElement inherits from SVGGraphicsElement in the first place (which is the reason this explicit IsFocusableInternal impl is needed here in the first place).  If you know the reason for that inheritance relationship, perhaps you could also add a brief comment above the "class SVGDefsElement final : public SVGGraphicsElement" line to hint at the reason?

::: dom/svg/SVGGraphicsElement.cpp
@@ +28,4 @@
>  
>  SVGGraphicsElement::~SVGGraphicsElement() {}
>  
> +bool SVGGraphicsElement::IsSVGFocusable(bool* aIsFocusable, int32_t* aTabIndex) {

This line is too long -- the args list needs to be wrapped here, to look like this:
bool SVGGraphicsElement::IsSVGFocusable(bool* aIsFocusable,
                                        int32_t* aTabIndex) {


("./mach clang-format" will actually make that change for you on its own, if you've got it configured. That's how I found that this needed to happen. :))

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9031765 [details] [diff] [review]
Part 1 - Move code to SVGGraphicsElement but special case <defs>

Review of attachment 9031765 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/svg/interact/scripted/tabindex-focus-flag.svg.ini
@@ +21,3 @@
>      expected: FAIL
>  
> +  [unknown[tabindex\] should be focusable.]

Also, it looks like these "should be focusable" failure annotations are new in this patch. (for "mesh" and "unknown")

Can you be sure we've got bugs tracking those new failures? (Or, maybe the "Part 2" patch that you mentioned in comment 1 will address these?)

Comment 5

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

> Also: it seems bizarre that SVGDefsElement inherits from SVGGraphicsElement
> in the first place (which is the reason this explicit IsFocusableInternal
> impl is needed here in the first place).  If you know the reason for that
> inheritance relationship, perhaps you could also add a brief comment above
> the "class SVGDefsElement final : public SVGGraphicsElement" line to hint at
> the reason?

That's from the SVG 2 spec. The spec is just odd here.

Comment 6

[Robert Longson [:longsonr]]

> 
> Can you be sure we've got bugs tracking those new failures? (Or, maybe the
> "Part 2" patch that you mentioned in comment 1 will address these?)

Nope, we simply don't support SVGUnknownElement (no browser does), nor do we support mesh elements. When all elements were focusable they got the default nsSVGElement focusable behaviour and they still do but that has changed.

bug 1239218 is for SVGUnknownElement, bug 1238882 is for mesh. Those bugs would implement support in passing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/608ea24efe57
Only graphics elements should support tabindex r=dholbert

Comment 8

[Robert Longson [:longsonr]]

Attachment: Part 2 - support the focusable attribute

Comment 9

[Robert Longson [:longsonr]]

Attachment: Part 3 - make IsSVGFocusable non-virtual

Comment 10

[Robert Longson [:longsonr]]

Attachment: Part 2 - support the focusable attribute

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/608ea24efe57

Comment 12

[Daniel Holbert [:dholbert]]

As I look at part 2, I'm realizing that the return value from this function...
> virtual bool IsSVGFocusable(bool* aIsFocusable, int32_t* aTabIndex);
https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/dom/svg/SVGGraphicsElement.h#28
...is non-intuitive. Particularly for cases where we do:
  *aIsFocuable = false;
  return true;
...like in the code being extended in part 2.

Could you add some documentation to SVGGraphicsElement.h to indicate what the return value means and when/whether callers need to bother checking it?  (At first glance, I'm seeing one caller that does check it, and one that does not.)

Comment 13

[Robert Longson [:longsonr]]

as far as I can tell with the return value true means "this is a definitive result" or and false means "caller might override this"

Comment 14

[Robert Longson [:longsonr]]

Attachment: Part 3 - make IsSVGFocusable non-virtual (with review comment)

I've added your part 2 review comment into part 3 as it was easier that way.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 9031994 [details] [diff] [review]
Part 2 - support the focusable attribute

Review of attachment 9031994 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the moment due to the focusable=false concern:

::: dom/svg/SVGGraphicsElement.cpp
@@ +42,5 @@
>  
> +  const nsAttrValue* val =
> +      mAttrs.GetAttr(nsGkAtoms::focusable, kNameSpaceID_None);
> +  if (val && val->Equals(nsGkAtoms::_false, eCaseMatters)) {
> +    *aIsFocusable = false;

So you're giving special significance to focusable="false" here (making it "win" and force something to be non-focusable, when it otherwise would've been treated as focusable due to e.g. a "tabindex" being provided.)

I don't see support for this focusable="false" behavior in the spec, though. https://www.w3.org/TR/SVG2/interact.html#Focus *only* mentions behavior for elements with focusable=true (they are considered to be focusable), and it doesn't mention any special semantics for "false".

Given that the spec text seems to be a halfhearted attempt at backwards-compatibility & that it suggests authors avoid this attribute altogether, it doesn't sound like this attr is meant to be fully-featured.  So we should probably omit this part (or get the spec updated to clarify who "wins" between focusable=false & tabindex=0)

@@ +53,5 @@
>      *aTabIndex = tabIndex;
>    }
>  
> +  // If a tabindex is specified at all, or the default tabindex is 0,
> +  // or focusable="true" we're focusable

Let's put a colon (or a comma) after "true" so that this sentence is easier to read.

i.e.
 "If A, or B, or C: we're focusable" 
rather than
  "...or C we're focusable"

Comment 16

[Robert Longson [:longsonr]]

OK, mind reviewing part 3 anyway, it's independent of part 2. If we want to do focusable we can do it in another bug.

Comment 17

[Robert Longson [:longsonr]]

Comment on attachment 9031994 [details] [diff] [review]
Part 2 - support the focusable attribute

>diff --git a/dom/svg/SVGGraphicsElement.cpp b/dom/svg/SVGGraphicsElement.cpp
>--- a/dom/svg/SVGGraphicsElement.cpp
>+++ b/dom/svg/SVGGraphicsElement.cpp
>@@ -36,9 +36,14 @@ bool SVGGraphicsElement::IsSVGFocusable(
>     if (aTabIndex) {
>       *aTabIndex = -1;
>     }
>-
>     *aIsFocusable = false;
>+    return true;
>+  }
> 
>+  const nsAttrValue* val =
>+      mAttrs.GetAttr(nsGkAtoms::focusable, kNameSpaceID_None);
>+  if (val && val->Equals(nsGkAtoms::_false, eCaseMatters)) {
>+    *aIsFocusable = false;
>     return true;
>   }
> 
>@@ -48,10 +53,10 @@ bool SVGGraphicsElement::IsSVGFocusable(
>     *aTabIndex = tabIndex;
>   }
> 
>-  // If a tabindex is specified at all, or the default tabindex is 0, we're
>-  // focusable
>-  *aIsFocusable = tabIndex >= 0 || HasAttr(nsGkAtoms::tabindex);
>-
>+  // If a tabindex is specified at all, or the default tabindex is 0,
>+  // or focusable="true" we're focusable
>+  *aIsFocusable = tabIndex >= 0 || HasAttr(nsGkAtoms::tabindex) ||
>+                  (val && val->Equals(nsGkAtoms::_true, eCaseMatters));
>   return false;
> }
> 
>diff --git a/testing/web-platform/meta/svg/interact/scripted/tabindex-focus-flag.svg.ini b/testing/web-platform/meta/svg/interact/scripted/tabindex-focus-flag.svg.ini
>--- a/testing/web-platform/meta/svg/interact/scripted/tabindex-focus-flag.svg.ini
>+++ b/testing/web-platform/meta/svg/interact/scripted/tabindex-focus-flag.svg.ini
>@@ -1,10 +1,4 @@
> [tabindex-focus-flag.svg]
>-  [rect[focusable=true\] should be focusable.]
>-    expected: FAIL
>-
>-  [a[focusable=false\] should not be focusable.]
>-    expected: FAIL
>-
>   [iframe should be focusable.]
>     expected: FAIL
> 
>--- a/xpcom/ds/StaticAtoms.py
>+++ b/xpcom/ds/StaticAtoms.py
>@@ -433,6 +433,7 @@ STATIC_ATOMS = [
>     Atom("floor", "floor"),
>     Atom("flowlength", "flowlength"),
>     Atom("focus", "focus"),
>+    Atom("focusable", "focusable"),
>     Atom("focused", "focused"),
>     Atom("followanchor", "followanchor"),
>     Atom("following", "following"),

Comment 18

[Daniel Holbert [:dholbert]]

So here's what I'm gleaning while trying to grok part 3:
 - IsSVGFocusable only exists to be a helper for the IsFocusableInternal API
 - Right now, SVGAElement gives itself some special behavior by overriding IsSVGFocusable
 - ...but instead it can just directly override the higher-level API.
 - And then, the IsSVGFocusable helper doesn't have any overrides, so it can be devirtualized.

Does that sum it up?

FWIW, it might be nice to capture some version of that ^^ (if it's correct) in the extended commit message, for the benefit of people staring at this commit in the future (e.g. regression-range-hunting)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 9032274 [details] [diff] [review]
Part 3 - make IsSVGFocusable non-virtual (with review comment)

Review of attachment 9032274 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine (and is a nice devirtualization win)

I wish we had more clearly-defined separation-of-concerns between the various functions here... but, this patch seems like a logical & valid evolution from where we're at right now.

Comment 20

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> So here's what I'm gleaning while trying to grok part 3:
>  - IsSVGFocusable only exists to be a helper for the IsFocusableInternal API
>  - Right now, SVGAElement gives itself some special behavior by overriding
> IsSVGFocusable
>  - ...but instead it can just directly override the higher-level API.
>  - And then, the IsSVGFocusable helper doesn't have any overrides, so it can
> be devirtualized.
> 
> Does that sum it up?
> 
> FWIW, it might be nice to capture some version of that ^^ (if it's correct)
> in the extended commit message, for the benefit of people staring at this
> commit in the future (e.g. regression-range-hunting)

That's it. I'll add an extended commit message as you suggest. 

I too am unhappy with how its designed but this is as far as I got in fixing it.

Comment 21

[Robert Longson [:longsonr]]

Looks like we match Chrome and Safari now. Only Edge supports focusable, and Edge doesn't seem to do tabindex very well for SVG content.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5537adc555
devirtualise IsSVGFocusable by having SVGAElement (its only override) directly override the higher level IsFocusableInternal API r=dholbert

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5c5537adc555