Closed Bug 767843 Opened 9 years ago Closed 9 years ago

stop using nsIDOMHTMLOptionElement 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

(2 files)

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

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

::: accessible/src/html/HTMLSelectAccessible.cpp
@@ +234,5 @@
>  
>    // Are we selected?
> +  bool selected = false;
> +  nsHTMLOptionElement* option = nsHTMLOptionElement::FromContent(mContent);
> +  if (option && option->Selected()) {

I'd do
bool selected = option && option->Selected();

note, below you don't have a null check option element, perhaps you can do
bool selected = nsHTMLOptionElement::FromContent(mContent)->Selected();
(In reply to alexander :surkov from comment #2)
> Comment on attachment 636203 [details] [diff] [review]
> patch
> 
> Review of attachment 636203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/html/HTMLSelectAccessible.cpp
> @@ +234,5 @@
> >  
> >    // Are we selected?
> > +  bool selected = false;
> > +  nsHTMLOptionElement* option = nsHTMLOptionElement::FromContent(mContent);
> > +  if (option && option->Selected()) {
> 
> I'd do
> bool selected = option && option->Selected();

I guess you could do that, though I think you still need the if to change the state.

> note, below you don't have a null check option element, perhaps you can do
> bool selected = nsHTMLOptionElement::FromContent(mContent)->Selected();

actually, I'm pretty sure that code is, and has been busted for optgroups for a while.  So I should probably add a null check htere.
Comment on attachment 636203 [details] [diff] [review]
patch

It makes sense to switch reviewer to Alexander.
Attachment #636203 - Flags: review?(dbolter)
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > Comment on attachment 636203 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 636203 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/html/HTMLSelectAccessible.cpp
> > @@ +234,5 @@
> > >  
> > >    // Are we selected?
> > > +  bool selected = false;
> > > +  nsHTMLOptionElement* option = nsHTMLOptionElement::FromContent(mContent);
> > > +  if (option && option->Selected()) {
> > 
> > I'd do
> > bool selected = option && option->Selected();
> 
> I guess you could do that, though I think you still need the if to change
> the state.

sure
bool selected = bla;
if (selected) {}

:)

> actually, I'm pretty sure that code is, and has been busted for optgroups
> for a while.  So I should probably add a null check htere.

ok, I see
Attached patch patchSplinter Review
Attachment #637553 - Flags: review?(surkov.alexander)
Attachment #637553 - Flags: review?(surkov.alexander) → review+
https://hg.mozilla.org/mozilla-central/rev/b3ad8edbd748
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.