Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making class TokenImageSpan public #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sagar-waghmare-rsl
Copy link

  • Made class TokenImageSpan public so that it can be accessed from outside.
  • We need this class public so that we can call getSpans() on the EditText.
  • Right now, this class is accessible, but crashes at runtime on devices
    with API less than lollipop and which have multidex enabled.

- Made class TokenImageSpan public so that it can be accessed from outside.
- We need this class public so that we can call getSpans() on the EditText.
- Right now, this class is accessible, but crashes at runtime on devices
  with API less than lollipop and which have multidex enabled.
@mgod
Copy link
Contributor

mgod commented Jan 11, 2017

I'm happy to consider making this class public, but relying on the behavior of getSpans() is very likely to break and be buggy (I do terrible things to the spans for backwards compatibility). I'm hoping there's a different way to do what you want. Why do you need to call getSpans()?

@sagar-waghmare-rsl
Copy link
Author

I want to add accessibility support to TokenCompleteTextView. Right now, when we enable TalkBack we see that the added Token's are not read out. So, we have added a ExploreByTouchHelper AccessibilityDelegate to the derived class of TokenCompleteTextView. We need the TokenImageSpan to be accessible in the ExploreByTouchHelper methods - getVisibleVirtualViews, onPopulateNodeForVirtualView etc.
Do you want me to add the accessibility support to TokenCompleteTextView ?

@mgod
Copy link
Contributor

mgod commented Jan 11, 2017

I would love to have better accessibility support in TokenCompleteTextView. I did a first pass using getTextForAccessibility and onInitializeAccessibilityEvent, but at the time I was putting it together I couldn't find good docs on adding this kind of support.

@mgod mgod force-pushed the master branch 2 times, most recently from bdb918d to 6bc7cd4 Compare August 24, 2017 19:40
@lynamemi
Copy link

+1 for this, re: needing accessibility. Even just making the view public would help.

@mgod
Copy link
Contributor

mgod commented Jan 11, 2019

I think we really need to bake accessibility into the library rather than make TokenImageSpan public, but I'm not really sure what changes I would need to make to improve things. Does anyone have a good resource to look at for adding more accessibility support? Or some sample code that would help me understand what you would do if TokenImageSpan was made public?

I'm kind of out of my depth on this, but I'm really hesitant to make it seem like public access to TokenImageSpan is a good idea.

@lynamemi
Copy link

Without getting too lost in the weeds, I'll try to describe the example I see in the code base I'm referencing. They added a TokenCompleteTextViewTouchHelper class that extends ExploreByTouchHelper (docs). That class sets up a bunch of "virtual views", and that's why we're wanting access to the TokenImageSpan (or in my case, even getting access to the view would help for checking if span views are selected and grabbing their position for creating said virtual views, etc).

Then when it comes to token click events, we would need to plug in accessibility events from the ExploreByTouchHelper for each
click style. For example, in TokenImageSpan's onClick,

            switch (tokenClickStyle) {
                case Select:
                case SelectDeselect:
                    if (!view.isSelected()) {
                        clearSelections();
                        final int tokenIndex = getIndexForTokenImageSpan(this);
                        mTokenCompleteTextViewTouchHelper.invalidateVirtualView(tokenIndex);
                        mTokenCompleteTextViewTouchHelper.sendEventForVirtualView(tokenIndex,
                                AccessibilityEvent.TYPE_VIEW_CLICKED);
                        view.setSelected(true);
                        mTokenCompleteTextViewTouchHelper.sendEventForVirtualView(tokenIndex,
                                AccessibilityEvent.TYPE_VIEW_SELECTED);
                        break;

It's definitely not easy code to implement. If baking it in is too much, I think there are alternative API hooks that could help with customizing accessibility. For me the biggest challenges to finding a work around have been not being able to check if the TokenSpan view is selected and getting the view bounds.

@mgod
Copy link
Contributor

mgod commented Jan 12, 2019

Thanks for the feedback. I do want to make better accessibility support at least possible even if baking it into the library doesn't wind up making sense.

I can see how you would need access to the selected state of the view to implement this. I can easily see exposing that publicly in some useful way.

From looking at the docs, it seems like the core reason to need access to the view other than selected state is to use it for the getVirtualViewAt method to figure out which view corresponds with a touch event (I'm pretty sure everything else is handled by a virtual view hierarchy that is unrelated to the actual view?). Would it work to just expose a rectangle for each view/span that represents its last position?

The main reason I don't want to expose the view more is that the views are not actually part of the view hierarchy, they are only used for drawing. If you had access to view, I assume you would use something like getDrawingRect to figure out where the view is positioned? At the moment, the implementation would have all token views with the upper left corner at 0,0, not at their real position on screen.

@lynamemi
Copy link

I hadn't realized that about the view, but it makes sense!

I'm still new to ExploreByTouch also, but from what I've gleaned so far, I think your two suggestions of 1. exposing selected state and 2. exposing a rectangle for each view's last know position sound pretty spot on. If anything else comes up, I'll let you know. And let me know how else I can help.

@mgod mgod added this to the 4.0.0 milestone Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants