Closed Bug 767860 Opened 9 years ago Closed 9 years ago

stop using nsIDOMHTMLInputElement in a11y

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #636224 - Flags: review?(dbolter)
Attachment #636224 - Flags: review?(bzbarsky)
Comment on attachment 636224 [details] [diff] [review]
patch

Passing off to Mounir, since he knows this stuff better than I do.
Attachment #636224 - Flags: review?(bzbarsky) → review?(mounir)
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 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*).
Attachment #636224 - Flags: review?(mounir) → review+
(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 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.
Attachment #636224 - Flags: review?(dbolter) → review+
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 :).)
(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
We could possibly have the biding code use Foo() instead of GetFoo() for infallible getters...  Worth a bug on that.
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.
(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?
I posted to .platform.
(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?
(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?
https://hg.mozilla.org/mozilla-central/rev/ca08bbcd7dc4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.