767843 - stop using nsIDOMHTMLOptionElement in a11y

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

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();

Comment 3

[Trevor Saunders (:tbsaunde)]

(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 4

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

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

It makes sense to switch reviewer to Alexander.

Comment 5

[alexander :surkov (:asurkov)]

(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

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 7

[Trevor Saunders (:tbsaunde)]

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

Comment 8

[Trevor Saunders (:tbsaunde)]

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

Comment 9

[Ed Morley [:emorley]]

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