Closed
Bug 751496
Opened 13 years ago
Closed 13 years ago
get rid nsAccessible::GetBoundsFrame
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
|
13.07 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #620620 -
Flags: review?(trev.saunders)
|
Assignee | |
Updated•13 years ago
|
Summary: get rid nsAccessible::GetBoundaryFrame → get rid nsAccessible::GetBoundsFrame
|
||
Comment 1•13 years ago
|
||
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•13 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.
|
|
||
Comment 3•13 years ago
|
||
I was confused I guess because I assumed that the test was related somehow since it is in the same patch. (?!)
|
|
Assignee | |
Comment 4•13 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•13 years ago
|
||
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 6•13 years ago
|
||
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•13 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•13 years ago
|
||
Target Milestone: --- → mozilla15
|
||
Comment 9•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•