Closed
Bug 767860
Opened 13 years ago
Closed 13 years ago
stop using nsIDOMHTMLInputElement in a11y
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
10.57 KB,
patch
|
davidb
:
review+
mounir
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #636224 -
Flags: review?(dbolter)
Attachment #636224 -
Flags: review?(bzbarsky)
![]() |
||
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 5•13 years ago
|
||
(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•13 years ago
|
||
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 7•13 years ago
|
||
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 :).)
Assignee | ||
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
We could possibly have the biding code use Foo() instead of GetFoo() for infallible getters... Worth a bug on that.
![]() |
||
Comment 10•13 years ago
|
||
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.
Assignee | ||
Comment 11•13 years ago
|
||
(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•13 years ago
|
||
I posted to .platform.
Assignee | ||
Comment 13•13 years ago
|
||
Comment 14•13 years ago
|
||
(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?
Assignee | ||
Comment 15•13 years ago
|
||
Assignee | ||
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•