1486671 - stop using nsIDOMXULCheckboxElement

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[James Teh [:Jamie]]

Comment on attachment 9004439 [details] [diff] [review]
patch

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

Since we're getting rid of XUL specific interfaces, I tend to think the correct solution here long-term is to make the XUL widgets use ARIA, rather than maintaining specific C++ code in the a11y module. However, I guess that might not be reasonable right now.

I realise some of this is largely copy/paste, so the nits are more nits in the old code. Thus, I won't complain too much if you don't address them, but figured I'd flag them while I was here.

::: accessible/base/XULMap.h
@@ +4,4 @@
>  
>  XULMAP_TYPE(browser, OuterDocAccessible)
>  XULMAP_TYPE(button, XULButtonAccessible)
> +XULMAP_TYPE(checkbox, CheckboxAccessible)

I'm curious as to why you combined HTML and XUL check boxes into one class. I'm not necessarily saying I think this is a "bad" thing, but it does mean we have unnecessary checks happening at runtime every time we query states. In practice, this probably amounts to nothing. If this were entirely new code, I probably wouldn't have even raised it, but in this case, we already had separate classes, so this was actually a lot more effort.

::: accessible/generic/Accessible.h
@@ +910,5 @@
>    /**
>     * Return true if the accessible state change is processed by handling proper
> +   * DOM UI event, if otherwise then false. For example, CheckboxAccessible
> +   * created for HTML:input@type="checkbox" will process
> +   * nsIDocumentObserver::ContentStateChanged instead 'CheckboxStateChange'

nit: "instead" -> "instead of"

::: accessible/generic/FormControlAccessible.cpp
@@ +161,5 @@
> +
> +void
> +CheckboxAccessible::ActionNameAt(uint8_t aIndex, nsAString& aName)
> +{
> +  if (aIndex == eAction_Click) {    // 0 is the magic value for default action

nit: We probably don't need this magic value comment, since we use a constant, not literal 0.

@@ +176,5 @@
> +
> +bool
> +CheckboxAccessible::DoAction(uint8_t aIndex) const
> +{
> +  if (aIndex != 0) {

nit: 0 -> eAction_Click?

@@ +190,5 @@
> +  uint64_t state = LeafAccessible::NativeState();
> +
> +  state |= states::CHECKABLE;
> +  dom::HTMLInputElement* input = dom::HTMLInputElement::FromNode(mContent);
> +  if (input) { // HTLM:input@type="checkbox"

nit: "HTLM:" -> "HTML:"

@@ +197,5 @@
> +
> +    if (input->Checked())
> +      return state | states::CHECKED;
> +  }
> +  else { // XUL checkbox

nit: "} else {" should be on same line.

@@ +200,5 @@
> +  }
> +  else { // XUL checkbox
> +    nsAutoString checked;
> +    mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, checked);
> +    if (checked.EqualsLiteral("true")) {

Can you use nsAccUtils::IsDOMAttrTrue here?

@@ +201,5 @@
> +  else { // XUL checkbox
> +    nsAutoString checked;
> +    mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, checked);
> +    if (checked.EqualsLiteral("true")) {
> +      return state | states::MIXED;

Should this be states::CHECKED (not MIXED)?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #1)
> Since we're getting rid of XUL specific interfaces, I tend to think the
> correct solution here long-term is to make the XUL widgets use ARIA, rather
> than maintaining specific C++ code in the a11y module. However, I guess that
> might not be reasonable right now.

I agree with this, and I think we should really think about it now, or get the reasons why it is not done this way at tis stage. Because at the XBL binding level, much of XUL really is HTML. And today the correct way to extend HTML into better widgets is using ARIA and doing the accessibility in JS, rather than having to pile on even more XUL-specific C++ code. Back in the early 2000s, the C++ way was the right way to do it, in 2018, it definitely is not. ;)

Comment 3

[ExE Boss]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> (In reply to James Teh [:Jamie] from comment #1)
> > Since we're getting rid of XUL specific interfaces, I tend to think the
> > correct solution here long-term is to make the XUL widgets use ARIA, rather
> > than maintaining specific C++ code in the a11y module. However, I guess that
> > might not be reasonable right now.
> 
> I agree with this, and I think we should really think about it now, or get
> the reasons why it is not done this way at tis stage. Because at the XBL
> binding level, much of XUL really is HTML. And today the correct way to
> extend HTML into better widgets is using ARIA and doing the accessibility in
> JS, rather than having to pile on even more XUL-specific C++ code. Back in
> the early 2000s, the C++ way was the right way to do it, in 2018, it
> definitely is not. ;)

Also, I’m pretty sure that back in the early 2000s, piling on C++ code was the *only* way to do accessibility stuff as ARIA didn’t yet exist.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> (In reply to James Teh [:Jamie] from comment #1)
> > Since we're getting rid of XUL specific interfaces, I tend to think the
> > correct solution here long-term is to make the XUL widgets use ARIA, rather
> > than maintaining specific C++ code in the a11y module. However, I guess that
> > might not be reasonable right now.
> 
> I agree with this, and I think we should really think about it now, or get
> the reasons why it is not done this way at tis stage. Because at the XBL
> binding level, much of XUL really is HTML. And today the correct way to
> extend HTML into better widgets is using ARIA and doing the accessibility in
> JS, rather than having to pile on even more XUL-specific C++ code. Back in
> the early 2000s, the C++ way was the right way to do it, in 2018, it
> definitely is not. ;)

Does aria work properly in XUL elements / XUL documents?

Also, do you know if it could cover the cases that we use from C++ for nsIDOMXULLabeledControlElement (https://searchfox.org/mozilla-central/search?q=nsIDOMXULLabeledControlElement&path=) and nsIDOMXULControlElement (https://searchfox.org/mozilla-central/search?q=nsIDOMXULControlElement&path=)?

If so we could avoid wiring up XPCOM with a huge chunk of the remaining bindings (everything under this node in the tree https://bgrins.github.io/xbl-analysis/tree/#basecontrol).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #1)

> Since we're getting rid of XUL specific interfaces, I tend to think the
> correct solution here long-term is to make the XUL widgets use ARIA, rather
> than maintaining specific C++ code in the a11y module. However, I guess that
> might not be reasonable right now.

imo, not worth it. XUL maintenance cost is reasonably low: it has a tiny logic comparing to other a11y code. Agreed that ARIA bribes by its universality and simplicity, but until you wrap all ARIA logic into custom elements and such, you may end up with a higher maintenance fee than XUL code we have. Also XUL may not stand forever, in which case transition to ARIA may be nothing but extra cost.

> I realise some of this is largely copy/paste, so the nits are more nits in
> the old code. Thus, I won't complain too much if you don't address them, but
> figured I'd flag them while I was here.

sure, I'm good to fix nits as we go.

> ::: accessible/base/XULMap.h
> @@ +4,4 @@
> >  
> >  XULMAP_TYPE(browser, OuterDocAccessible)
> >  XULMAP_TYPE(button, XULButtonAccessible)
> > +XULMAP_TYPE(checkbox, CheckboxAccessible)
> 
> I'm curious as to why you combined HTML and XUL check boxes into one class.
> I'm not necessarily saying I think this is a "bad" thing, but it does mean
> we have unnecessary checks happening at runtime every time we query states.
> In practice, this probably amounts to nothing. If this were entirely new
> code, I probably wouldn't have even raised it, but in this case, we already
> had separate classes, so this was actually a lot more effort.

The primary reason is a lower maintenance fee. Less code, less code paths to test and fix. Also there's no runtime penalty I believe because no new checks were added (in case of HTML, which 99% of all use cases I believe). Also it'd be easier to remove XUL bits, if XUL falls after XBL.

> > +  else { // XUL checkbox
> > +    nsAutoString checked;
> > +    mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, checked);
> > +    if (checked.EqualsLiteral("true")) {
> 
> Can you use nsAccUtils::IsDOMAttrTrue here?

if you don't mind I switch to direct Element::AttrValueIs() that nsAccUtils::IsDOMAttrTrue uses, it is one line of code to benefit from wrapping (which is not inline btw).

> 
> @@ +201,5 @@
> > +  else { // XUL checkbox
> > +    nsAutoString checked;
> > +    mContent->AsElement()->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, checked);
> > +    if (checked.EqualsLiteral("true")) {
> > +      return state | states::MIXED;
> 
> Should this be states::CHECKED (not MIXED)?

yes

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Marco Zehe (:MarcoZ) from comment #2)
> > (In reply to James Teh [:Jamie] from comment #1)
> > > Since we're getting rid of XUL specific interfaces, I tend to think the
> > > correct solution here long-term is to make the XUL widgets use ARIA, rather
> > > than maintaining specific C++ code in the a11y module. However, I guess that
> > > might not be reasonable right now.
> > 
> > I agree with this, and I think we should really think about it now, or get
> > the reasons why it is not done this way at tis stage. Because at the XBL
> > binding level, much of XUL really is HTML. And today the correct way to
> > extend HTML into better widgets is using ARIA and doing the accessibility in
> > JS, rather than having to pile on even more XUL-specific C++ code. Back in
> > the early 2000s, the C++ way was the right way to do it, in 2018, it
> > definitely is not. ;)
> 
> Does aria work properly in XUL elements / XUL documents?

it is

> Also, do you know if it could cover the cases that we use from C++ for
> nsIDOMXULLabeledControlElement
> (https://searchfox.org/mozilla-central/
> search?q=nsIDOMXULLabeledControlElement&path=) and nsIDOMXULControlElement
> (https://searchfox.org/mozilla-central/
> search?q=nsIDOMXULControlElement&path=)?

ARIA can cover those for sure, it'd be a work. Also not quite sure, if it will make a right use case for ARIA, since ARIA is designed to make custom things accessible, which the browser knows nothing about. So ARIA will lead to the semantics duplication, for example, for each xul:label@for you will have to have a duplicate relation provided by aria-labelledby.

> If so we could avoid wiring up XPCOM with a huge chunk of the remaining
> bindings (everything under this node in the tree
> https://bgrins.github.io/xbl-analysis/tree/#basecontrol).

If only a11y needs it, then a11y can handle it on its own I think. But please let's file another bug to keep the discussion on.

Comment 7

[James Teh [:Jamie]]

(In reply to ExE Boss from comment #3)
> Also, I’m pretty sure that back in the early 2000s, piling on C++ code was
> the *only* way to do accessibility stuff as ARIA didn’t yet exist.

That's quite correct. No one was attempting to suggest otherwise, but my apologies if it seemed that way.

(In reply to alexander :surkov (:asurkov) from comment #5)
> imo, not worth it. XUL maintenance cost is reasonably low: it has a tiny
> logic comparing to other a11y code. Agreed that ARIA bribes by its
> universality and simplicity, but until you wrap all ARIA logic into custom
> elements and such, you may end up with a higher maintenance fee than XUL
> code we have.

Sure, but for something like this, the JS logic should be fairly simple. It also makes the integration between the XUL widgets and their accessibility tighter, which is a good thing in the long-run.

Having said that, I acknowledge that we do have people on the a11y team who are familiar with this code and can make the changes, whereas we don't necessarily have people who know the front-end code well enough to make the required changes there in the same time frame. That is definitely a consideration here if the a11y team is going to get tasked with this work.

> Also XUL may not stand forever, in which case transition to
> ARIA may be nothing but extra cost.

my understanding (as per bug 1455433 comment 4) is that these new custom elements are likely to be around for a while, rather than rewriting the front-end code:

> For bindings that have a lot of frontend consumers we've generally attempted to provide a migration to Custom Element that requires as little as possible change to the calling code.

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Does aria work properly in XUL elements / XUL documents?

Yes. There are some edge cases which might not be covered (e.g. bug 1480058), but we should just fix those (as we did this one) as needed.

(In reply to alexander :surkov (:asurkov) from comment #6)
> Also not quite sure, if it
> will make a right use case for ARIA, since ARIA is designed to make custom
> things accessible, which the browser knows nothing about.

As I understand it, with custom elements, the browser doesn't know about XUL elements either. The implementation is done in HTML, CSS, etc. A11y is no different here: the custom element handles the translation to the right HTML, CSS and ARIA.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

with nits addressed

Comment 9

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #7)

> (In reply to alexander :surkov (:asurkov) from comment #6)
> > Also not quite sure, if it
> > will make a right use case for ARIA, since ARIA is designed to make custom
> > things accessible, which the browser knows nothing about.
> 
> As I understand it, with custom elements, the browser doesn't know about XUL
> elements either. The implementation is done in HTML, CSS, etc. A11y is no
> different here: the custom element handles the translation to the right
> HTML, CSS and ARIA.

Correct, my point though is a custom element will have same tag name of same namespace, and we already know how to expose XUL to the assistive technologies, and thus no changes is required on accessibility side.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0af181b51b76
stop using nsIDOMXULCheckboxElement, r=jamie

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0af181b51b76