stop using nsIDOMHTMLOptionElement in a11y

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 636203 [details] [diff] [review]
patch
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();
(Assignee)

Comment 3

6 years ago
(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
(Assignee)

Comment 6

6 years ago
Created attachment 637553 [details] [diff] [review]
patch
Attachment #637553 - Flags: review?(surkov.alexander)
Attachment #637553 - Flags: review?(surkov.alexander) → review+

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/b3ad8edbd748
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.