Closed Bug 1286952 Opened 3 years ago Closed 3 years ago

Make xpcAccessibleHyperText work with proxied accessibles.

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: michael.li11702, Assigned: michael.li11702)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
ProxyAccessible doesn't have versions of HyperTextAccessible's EnclosingRange, SelectionRanges, VisibleRanges, RangeAtChild, and RangeAtPoint functions, so those portions of the code aren't changed.
Attachment #8771106 - Flags: review?(tbsaunde+mozbugs)
Summary: Make xpcAccessibleHyperText work with proxied accessibles. r?tbsaunde → Make xpcAccessibleHyperText work with proxied accessibles.
Assignee: nobody → mili
Comment on attachment 8771106 [details] [diff] [review]
Make xpcAccessibleHyperText work with proxied accessibles. r?tbsaunde

>   %{C++
>   virtual mozilla::a11y::Accessible* ToInternalAccessible() const = 0;
>+  virtual mozilla::a11y::ProxyAccessible* ToInternalProxy() const = 0;

yuk, I don't know why that is there, but lets not add to it.

> xpcAccessibleHyperText::GetCharacterCount(int32_t* aCharacterCount)

This part seems fine, so you could split it out.

> xpcAccessibleHyperText::GetText(int32_t aStartOffset, int32_t aEndOffset,
>                                 nsAString& aText)
>+  } else {
>+    nsString text(aText);

its only an out argument there's no point in copying it into text here.

> xpcAccessibleHyperText::GetTextBeforeOffset(int32_t aOffset,
>                                             AccessibleTextBoundary aBoundaryType,
>                                             int32_t* aStartOffset,
>                                             int32_t* aEndOffset,
>                                             nsAString& aText)
> {
>+    nsString text(aText);

same

> xpcAccessibleHyperText::GetTextAtOffset(int32_t aOffset,
>                                         AccessibleTextBoundary aBoundaryType,
>                                         int32_t* aStartOffset,
>                                         int32_t* aEndOffset, nsAString& aText)
> {
>+  } else {
>+    nsString text(aText);

and here

> xpcAccessibleHyperText::GetTextAfterOffset(int32_t aOffset,
>                                            AccessibleTextBoundary aBoundaryType,
>                                            int32_t* aStartOffset,
>                                            int32_t* aEndOffset, nsAString& aText)
> {
>+  } else {
>+    nsString text(aText);

and here

> xpcAccessibleHyperText::GetTextAttributes(bool aIncludeDefAttrs,
>                                           int32_t aOffset,
>                                           int32_t* aStartOffset,
>                                           int32_t* aEndOffset,
>                                           nsIPersistentProperties** aAttributes)
> {
>+  nsCOMPtr<nsIPersistentProperties> props = 
>+		do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);

That's a waste of time in the case mIntl is an accessible.

>+  props.swap(*aAttributes);

might as well switch to .forget while here

>+  nsCOMPtr<nsIPersistentProperties> props = 
>+		do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);

same as previous method

> xpcAccessibleHyperText::GetSelectionBounds(int32_t aSelectionNum,
>                                            int32_t* aStartOffset,
>                                            int32_t* aEndOffset)
> {
>+    if (aSelectionNum >= mIntl.AsProxy()->SelectionCount())

again  I'd rather not make two ipc calls in one method.

> xpcAccessibleHyperText::GetSelectionRanges(nsIArray** aRanges)
> {
>-  if (!Intl())
>+  if (mIntl.IsNull())

there doesn't really seem to be a point to this change at this point?

> xpcAccessibleHyperText::GetVisibleRanges(nsIArray** aRanges)
> {
>   NS_ENSURE_ARG_POINTER(aRanges);
>   *aRanges = nullptr;
> 
>-  if (!Intl())
>+  if (mIntl.IsNull())

same

>-  if (!Intl())
>+  if (mIntl.IsNull())

and here

>-  if (!Intl())
>+  if (mIntl.IsNull())

and here

> xpcAccessibleHyperText::GetLinkIndex(nsIAccessibleHyperLink* aLink,
>                                      int32_t* aIndex)
> {
>   nsCOMPtr<nsIAccessible> xpcLink(do_QueryInterface(aLink));
>-  Accessible* link = xpcLink->ToInternalAccessible();
>-  if (link)
>-    *aIndex = Intl()->LinkIndexOf(link);
>+  if (Accessible* accLink = xpcLink->ToInternalAccessible()) {
>+    *aIndex = Intl()->LinkIndexOf(accLink);
>+  } else if (ProxyAccessible* proxyLink = xpcLink->ToInternalProxy()) {

ok, this is pretty silly.  I'd say the thing to do here is write some separate patches to make all the nsIAccessible* interfaces builtinclass, and then you can change this to just static cast from nsIAccessibleHyperLink* to xpcAccessibleGeneric*.
Attachment #8771106 - Flags: review?(tbsaunde+mozbugs)
Depends on: 1288508
This patch assumes the patch from bug 1288508 has been applied.
Attachment #8773447 - Flags: review?(tbsaunde+mozbugs)
Attachment #8771106 - Attachment is obsolete: true
Fixed a missing bang operator in proxy's if block of SetSelectionBounds()
Attachment #8773453 - Flags: review?(tbsaunde+mozbugs)
Attachment #8773447 - Attachment is obsolete: true
Attachment #8773447 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8773453 [details] [diff] [review]
Make xpcAccessibleHyperText work with proxied accessibles.

>+  if (mIntl.IsAccessible()) {
>+    Intl()->TextAfterOffset(aOffset, aBoundaryType, aStartOffset, aEndOffset, 
>+                            aText);
>+  } else {
>+    nsString text(aText);

it doesn't need to be initialized to aText.

> NS_IMETHODIMP
> xpcAccessibleHyperText::GetLinkIndex(nsIAccessibleHyperLink* aLink,
>                                      int32_t* aIndex)
> {

it would have been better to split this out since its kind of complicated, but anyway.

>   nsCOMPtr<nsIAccessible> xpcLink(do_QueryInterface(aLink));
>-  Accessible* link = xpcLink->ToInternalAccessible();
>-  if (link)
>-    *aIndex = Intl()->LinkIndexOf(link);
>+  if (Accessible* accLink = xpcLink->ToInternalAccessible()) {
>+    *aIndex = Intl()->LinkIndexOf(accLink);
>+  } else {
>+    xpcAccessibleGeneric* linkGeneric =
>+      static_cast<xpcAccessibleGeneric*>(xpcLink.get());

the QueryInterface to nsIAccessible is kind of a waste of time in this case, but I guess its fine for now.

>+    xpcAccessibleHyperText* linkHyperText =
>+      static_cast<xpcAccessibleHyperText*>(linkGeneric);

Why is this needed?
Attachment #8773453 - Flags: review?(tbsaunde+mozbugs) → review+
Fixed stuff from comment 5 and adjusted lines in ScrollSubstringToPoint() that were over 80 characters.
Attachment #8773837 - Flags: review+
Attachment #8773453 - Attachment is obsolete: true
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebased with central.
Attachment #8775248 - Flags: review+
Attachment #8773837 - Attachment is obsolete: true
Keywords: checkin-needed
Still doesn't apply :(

patching file accessible/xpcom/xpcAccessibleHyperText.cpp
Hunk #1 FAILED at 35
Hunk #2 FAILED at 194
Hunk #3 FAILED at 446
3 out of 3 hunks FAILED -- saving rejects to file accessible/xpcom/xpcAccessibleHyperText.cpp.rej
Keywords: checkin-needed
Attachment #8775248 - Attachment is obsolete: true
Keywords: checkin-needed
The attached patch is still broken, but I was able to import it from your Try push anyway.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf6ee84ac6f
Make xpcAccessibleHyperText work with proxied accessibles. r=tbsaunde
Keywords: checkin-needed
I'll try using moz review more, this is the second time a patch hasn't been able to apply.. not sure what the issue is.
https://hg.mozilla.org/mozilla-central/rev/0bf6ee84ac6f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.