-
-
Notifications
You must be signed in to change notification settings - Fork 242
Accessibility Inspector: Allow navigation in all possible directions #630
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
base: main
Are you sure you want to change the base?
Conversation
// Direction represents navigation direction values used by AX service | ||
const ( | ||
DirectionPrevious int32 = 3 | ||
DirectionNext int32 = 4 | ||
DirectionFirst int32 = 5 | ||
DirectionLast int32 = 6 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use a custom type to represent the direction to make it a bit more clear that Move
doesn't take any random integer valuye
// Direction represents navigation direction values used by AX service | |
const ( | |
DirectionPrevious int32 = 3 | |
DirectionNext int32 = 4 | |
DirectionFirst int32 = 5 | |
DirectionLast int32 = 6 | |
) | |
type MoveDirection int32 | |
// Direction represents navigation direction values used by AX service | |
const ( | |
DirectionPrevious MoveDirection(3) | |
DirectionNext MoveDirection(4) | |
DirectionFirst MoveDirection(5) | |
DirectionLast MoveDirection(6) | |
) |
// GetElement moves the green selection rectangle one element further | ||
func (a ControlInterface) GetElement() { | ||
// Move navigates focus using the given direction and returns selected fields as a map. | ||
func (a ControlInterface) Move(direction int32) map[string]interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the contents of the map that we return here? It would be nice to return a struct here to make that less error prone
msg, err := a.channel.ReceiveMethodCallWithTimeout("hostInspectorCurrentElementChanged:", timeout) | ||
if err != nil { | ||
log.Errorf("Timeout waiting for hostInspectorCurrentElementChanged (timeout: %v): %v", timeout, err) | ||
panic(fmt.Sprintf("Timeout waiting for hostInspectorCurrentElementChanged: %s", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't use panics. And why a 2s timeout, is this unreliable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to add to the timeout. I think that's a case where context.Context
could/should be used, and then it's up to the caller how long that context is valid, and then how long we would wait at most
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i return an error now. Yeah I would reduce the timeout. It was arbitrary
Problem: It was only possible to navigate to next element
Solution
deviceInspectorMoveWithOptions
PlatformElementValue_V1
unique base64 identifier which could potentially be used to tap on element.ReceiveMethodCallWithTimeout
becauseReceiveMethodCall
expects the dtx message to be always arrived, reading channel inside ReceiveMethodCall becomes blocking foreverReproducible Steps for blocking ReceiveMethodCall:
hostInspectorCurrentElementChanged
doesn't receive dtx message because AX inspector doesn't find an element in accessibility tree.