Closed Bug 751496 Opened 12 years ago Closed 12 years ago

get rid nsAccessible::GetBoundsFrame

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

patch2
13.07 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review 
      No description provided.
Attachment #620620 - Flags: review?(trev.saunders)
Blocks: 751497
Summary: get rid nsAccessible::GetBoundaryFrame → get rid nsAccessible::GetBoundsFrame
Comment on attachment 620620 [details] [diff] [review]
patch

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

Cool. Does this actually fix some selection issues?

::: accessible/tests/mochitest/bounds/test_select.html
@@ +70,5 @@
> +    addA11yLoadEvent(doTest);
> +  </script>
> +</head>
> +<body>
> +

Don't forget to mention this bug ;)
(In reply to David Bolter [:davidb] from comment #1)

> Cool. Does this actually fix some selection issues?

no, should it?

> Don't forget to mention this bug ;)

I didn't do that since it's just code cleanup.
I was confused I guess because I assumed that the test was related somehow since it is in the same patch. (?!)
(In reply to David Bolter [:davidb] from comment #3)
> I was confused I guess because I assumed that the test was related somehow
> since it is in the same patch. (?!)

it's related, I just added a testing for changed code paths.
Attached patch patch2 — — Splinter Review 
Assignee: nobody → surkov.alexander

        Attachment #620620 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #620620 -
        Flags: review?(trev.saunders)

        Attachment #620975 -
        Flags: review?(trev.saunders)

    
    

    
  
Comment on attachment 620975 [details] [diff] [review]
patch2

> ////////////////////////////////////////////////////////////////////////////////
>+// nsHTMLSelectOptionAccessible: nsAccessible protected
>+
>+void
>+nsHTMLSelectOptionAccessible::GetBoundsRect(nsRect& aTotalBounds,
>+                                            nsIFrame** aBoundingFrame)

not actually protected right?

btw plans to get rid of the out arg here?

>+{
>+  nsAccessible* combobox = GetCombobox();
>+  if (combobox && (combobox->State() & states::COLLAPSED)) {

wouldn't it be faster to check if the frame is expanded?

>+    combobox->GetBoundsRect(aTotalBounds, aBoundingFrame);
>+    return;
>+  }
>+
>+  nsHyperTextAccessibleWrap::GetBoundsRect(aTotalBounds, aBoundingFrame);

maybe just use else here?

>+    if (mParent && mParent->IsListControl()) {
>+      nsAccessible* combobox = mParent->Parent();
>+      return combobox->IsCombobox() ? combobox : nsnull;
>+    }
>+    return nsnull;

nit, I'd probably prefer a blank line after the }

>+

        Attachment #620975 -
        Flags: review?(trev.saunders) → review+

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> >+void
> >+nsHTMLSelectOptionAccessible::GetBoundsRect(nsRect& aTotalBounds,
> >+                                            nsIFrame** aBoundingFrame)
> btw plans to get rid of the out arg here?

I look into it to have something nice instead GetBoundsRect/GetBounds/conversion methods.

> >+  nsAccessible* combobox = GetCombobox();
> >+  if (combobox && (combobox->State() & states::COLLAPSED)) {
> 
> wouldn't it be faster to check if the frame is expanded?

there's ARIA I think (which should technically override it), anyway I prefer to rely on a11y logic until we clearly can use layout.

> >+    combobox->GetBoundsRect(aTotalBounds, aBoundingFrame);
> >+    return;
> >+  }
> >+
> >+  nsHyperTextAccessibleWrap::GetBoundsRect(aTotalBounds, aBoundingFrame);
> 
> maybe just use else here?

I think I tried this and that but ended up with this. If you like then I change it

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/bfe31dbdee16
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: