Last Comment Bug 751496 - get rid nsAccessible::GetBoundsFrame
: get rid nsAccessible::GetBoundsFrame
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: cleana11y 751497
  Show dependency treegraph
 
Reported: 2012-05-03 01:35 PDT by alexander :surkov
Modified: 2012-05-08 03:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.02 KB, patch)
2012-05-03 01:35 PDT, alexander :surkov
no flags Details | Diff | Review
patch2 (13.07 KB, patch)
2012-05-03 23:32 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description alexander :surkov 2012-05-03 01:35:55 PDT
Created attachment 620620 [details] [diff] [review]
patch
Comment 1 David Bolter [:davidb] 2012-05-03 05:57:52 PDT
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 ;)
Comment 2 alexander :surkov 2012-05-03 07:04:51 PDT
(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.
Comment 3 David Bolter [:davidb] 2012-05-03 07:11:22 PDT
I was confused I guess because I assumed that the test was related somehow since it is in the same patch. (?!)
Comment 4 alexander :surkov 2012-05-03 07:16:51 PDT
(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.
Comment 5 alexander :surkov 2012-05-03 23:32:34 PDT
Created attachment 620975 [details] [diff] [review]
patch2
Comment 6 Trevor Saunders (:tbsaunde) 2012-05-07 00:26:50 PDT
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 }

>+
Comment 7 alexander :surkov 2012-05-07 08:02:20 PDT
(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
Comment 9 Ed Morley [:emorley] 2012-05-08 03:23:39 PDT
https://hg.mozilla.org/mozilla-central/rev/bfe31dbdee16

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