The default bug view has changed. See this FAQ.

get rid nsAccessible::GetBoundsFrame

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

13.07 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 620620 [details] [diff] [review]
patch
Attachment #620620 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Blocks: 751497
(Assignee)

Updated

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

Comment 2

5 years ago
(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. (?!)
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 620975 [details] [diff] [review]
patch2
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+
(Assignee)

Comment 7

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

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe31dbdee16
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/bfe31dbdee16
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.