767860 - stop using nsIDOMHTMLInputElement in a11y

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Passing off to Mounir, since he knows this stuff better than I do.

Comment 3

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +92,5 @@
>  
>    state |= states::CHECKABLE;
> +  nsHTMLInputElement* input = nsHTMLInputElement::FromContent(mContent);
> +  if (!input)
> +    return state;

if it's not HTML input then it's not checkbox and it shouldn't be checkable. Either you don't need a check at all or return 0 if you want to be on safe side

Comment 4

[Mounir Lamouri (:mounir)]

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

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

r=me with the comments applied/answered.

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ -20,3 @@
>  #include "nsIDOMNSEditableElement.h"
> -#include "nsIDOMHTMLFormElement.h"
> -#include "nsIDOMHTMLLegendElement.h"

Why do you remove those includes?

@@ -23,2 @@
>  #include "nsIDOMHTMLTextAreaElement.h"
> -#include "nsIDOMNodeList.h"

and this one?

@@ +447,5 @@
>      mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::autocomplete,
>                        autocomplete);
>  
>      if (!autocomplete.LowerCaseEqualsLiteral("off")) {
> +      nsIContent* formContent = input->GetFormElement();

Can't |input| be null here?

::: content/html/content/src/nsHTMLInputElement.h
@@ +280,5 @@
>    bool DefaultChecked() const {
>      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> +  }
> +
> +  bool Indeterminate() const { return mIndeterminate; }

You can just use GetIndeterminate(bool*).

@@ +281,5 @@
>      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> +  }
> +
> +  bool Indeterminate() const { return mIndeterminate; }
> +  bool Checked() const { return mChecked; }

You can just use GetChecked(bool*).

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Comment on attachment 636224 [details] [diff] [review]
> patch
> 
> Review of attachment 636224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments applied/answered.
> 
> ::: accessible/src/html/HTMLFormControlAccessible.cpp
> @@ -20,3 @@
> >  #include "nsIDOMNSEditableElement.h"
> > -#include "nsIDOMHTMLFormElement.h"
> > -#include "nsIDOMHTMLLegendElement.h"
> 
> Why do you remove those includes?
> 
> @@ -23,2 @@
> >  #include "nsIDOMHTMLTextAreaElement.h"
> > -#include "nsIDOMNodeList.h"
> 
> and this one?

just removing stuff that isn't used anymore while I'm there anyway.

> @@ +447,5 @@
> >      mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::autocomplete,
> >                        autocomplete);
> >  
> >      if (!autocomplete.LowerCaseEqualsLiteral("off")) {
> > +      nsIContent* formContent = input->GetFormElement();
> 
> Can't |input| be null here?

its slightly tricky I admitt, but no because of the if at line 423.

> ::: content/html/content/src/nsHTMLInputElement.h
> @@ +280,5 @@
> >    bool DefaultChecked() const {
> >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > +  }
> > +
> > +  bool Indeterminate() const { return mIndeterminate; }
> 
> You can just use GetIndeterminate(bool*).
> 
> @@ +281,5 @@
> >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > +  }
> > +
> > +  bool Indeterminate() const { return mIndeterminate; }
> > +  bool Checked() const { return mChecked; }
> 
> You can just use GetChecked(bool*).

I could, but that would mean out args, and calling virtual functions.  Implementing the xpcom method with the inline though seems reasonable if you like.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

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

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

r=me if tests pass, but please reach agreement about the nsHTMLInputElement.h API and make sure Alexander his happy about his nit (probably return 0).

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +417,5 @@
>  
>    // Is it an <input> or a <textarea> ?
> +  nsHTMLInputElement* input = nsHTMLInputElement::FromContent(mContent);
> +  state |= input && input->IsSingleLineTextControl() ?
> +    states::SINGLE_LINE : states::MULTI_LINE;

Optional nit: I'd definitely prefer (input && input->blah) in brackets.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

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

::: content/html/content/src/nsHTMLInputElement.h
@@ +281,5 @@
>      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> +  }
> +
> +  bool Indeterminate() const { return mIndeterminate; }
> +  bool Checked() const { return mChecked; }

The new DOM bindings will expect functions like this, but called GetIndeterminate() and GetChecked(). It would be nice to just call them that now. (And yes, I know that's inconsistent with DefaultChecked(), because I added that one :).)

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to :Ms2ger from comment #7)
> Comment on attachment 636224 [details] [diff] [review]
> patch
> 
> Review of attachment 636224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLInputElement.h
> @@ +281,5 @@
> >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > +  }
> > +
> > +  bool Indeterminate() const { return mIndeterminate; }
> > +  bool Checked() const { return mChecked; }
> 
> The new DOM bindings will expect functions like this, but called
> GetIndeterminate() and GetChecked(). It would be nice to just call them that
> now. (And yes, I know that's inconsistent with DefaultChecked(), because I
> added that one :).)

I'd claim that's laime, and also inconsistant with the theory of GetFoo() being failable, but ok

Comment 9

[Boris Zbarsky [:bzbarsky]]

We could possibly have the biding code use Foo() instead of GetFoo() for infallible getters...  Worth a bug on that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Though note that for pointer return values, Foo() doesn't just mean infallible; it also means never returns null.  Of course we'd know that statically from the IDL too.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Though note that for pointer return values, Foo() doesn't just mean
> infallible; it also means never returns null.  Of course we'd know that
> statically from the IDL too.

ok, then I'll leave that as want me to file the binding gen bug or do one of you two want to just do it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I posted to .platform.

Comment 13

[Trevor Saunders (:tbsaunde)]

https://tbpl.mozilla.org/?tree=Try&rev=7135192653ea

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > ::: content/html/content/src/nsHTMLInputElement.h
> > @@ +280,5 @@
> > >    bool DefaultChecked() const {
> > >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > > +  }
> > > +
> > > +  bool Indeterminate() const { return mIndeterminate; }
> > 
> > You can just use GetIndeterminate(bool*).
> > 
> > @@ +281,5 @@
> > >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > > +  }
> > > +
> > > +  bool Indeterminate() const { return mIndeterminate; }
> > > +  bool Checked() const { return mChecked; }
> > 
> > You can just use GetChecked(bool*).
> 
> I could, but that would mean out args, and calling virtual functions. 
> Implementing the xpcom method with the inline though seems reasonable if you
> like.

I'm still not certain I understand why you wanted that. Was the code performance sensitive?

Comment 15

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca08bbcd7dc4

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Mounir Lamouri (:mounir) from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > ::: content/html/content/src/nsHTMLInputElement.h
> > > @@ +280,5 @@
> > > >    bool DefaultChecked() const {
> > > >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > > > +  }
> > > > +
> > > > +  bool Indeterminate() const { return mIndeterminate; }
> > > 
> > > You can just use GetIndeterminate(bool*).
> > > 
> > > @@ +281,5 @@
> > > >      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> > > > +  }
> > > > +
> > > > +  bool Indeterminate() const { return mIndeterminate; }
> > > > +  bool Checked() const { return mChecked; }
> > > 
> > > You can just use GetChecked(bool*).
> > 
> > I could, but that would mean out args, and calling virtual functions. 
> > Implementing the xpcom method with the inline though seems reasonable if you
> > like.
> 
> I'm still not certain I understand why you wanted that. Was the code
> performance sensitive?

I'd say somewhat so, certainly enough I'd rather not make things slower without a good reason.

I'm not really seeing a downside to them especially considering comment 7 am I missing something?

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca08bbcd7dc4