Open Bug 1148313 Opened 7 years ago Updated 7 years ago

[ScreenReader] Highlight box rendering problem while scrolling the screen


(Core :: Disability Access APIs, defect)

Gonk (Firefox OS)
Not set




(Reporter: kechen, Unassigned)



(3 files)

This case occurs when the highlight box is located in an iframe of a mozbrowser frame.

In this situation, when scrolling or pinch the screen the highlight box will repaint in the mozbrowser frame rather than the original position in iframe.
In Bug 1113494, we try to extend the usage of screenReader. To achieve these functionalities, we want to use original touch acts to control the UI which, however, are blocked by PointerAdapter.jsm[1].

So in this patch we hide the PointerAdapter.jsm to avoid the event blocking.

But there are some problems when we doing this. This bug is one of them.

In this scenario, we choose an element in mozbrowser first, and then choose another element in the iframe embedded in the mozbrowser and the screenReader will draw the highlight box on it. After that, we do the touch acts. The following are the problems that we encounter:

1. Pinch zoom : When do the pinch zoom act, the highlight box disappear in the iframe and is redrawn on the mozbrowser element.

2. Scroll : If scrolling in the iframe, it will react normally, but when scrolling out of the iframe it will react like the pinch zoom case above.

In my opinion, it is more make sense to let the highlight box stay in iframe when doing above acts. The original code won't reach these case, but I think it is still a bug existing in the code.

These two touch gestures will dispatch a 'scroll' event and trigger the Presentation.viewportChanged at EventManager.jsm[1]. 
And the highlight box's position will change according to 'scroll' event's target.

In this patch, I store the previous focused window when selecting the element and update the highlight box's position according to this value. This will remain the highlight box to stay in original element.

And the 'resize' event will trigger the viewport change and shouldn't move the highlight box, too. So I also modify this act.

Flags: needinfo?(schien)
Comment on attachment 8585958 [details] [diff] [review]
Fix the position of highlight box in iframe when view port change

Review of attachment 8585958 [details] [diff] [review]:

::: accessible/jsat/Presentation.jsm
@@ +156,5 @@
>  VisualPresenter.prototype.BORDER_PADDING = 2;
>  VisualPresenter.prototype.viewportChanged =
> +  function VisualPresenter_viewportChanged() {
> +    let currentDisplay = this._displayedAccessibles.get(this._currentWindow);

Considering scrolling on a parallel iframe, we'll send an extra IPC message that triggers no visual update. I'm wondering if there is any precise way to know the viewport change on pivot element.

@@ +427,5 @@
>          text: [],
> +        scrollX: this._currentWindow.scrollX,
> +        scrollY: this._currentWindow.scrollY,
> +        maxScrollX: this._currentWindow.scrollMaxX,
> +        maxScrollY: this._currentWindow.scrollMaxY

|_currentWindow| property doesn't existed in AndroidPresenter object, so this change is probably not correct.
Flags: needinfo?(schien)
Comment on attachment 8585852 [details] [diff] [review]
Block PointerAdapter.jsm

Review of attachment 8585852 [details] [diff] [review]:

::: accessible/jsat/AccessFu.jsm
@@ +124,5 @@
>      this.Input.start();
>      Output.start();
> +    //PointerAdapter.start();

I hope this is not an actual change you are proposing :)

Maybe this could be exposed as a preference?
(In reply to Eitan Isaacson [:eeejay] from comment #4)
> Comment on attachment 8585852 [details] [diff] [review]
> Block PointerAdapter.jsm
> Review of attachment 8585852 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: accessible/jsat/AccessFu.jsm
> @@ +124,5 @@
> >  
> >  
> >      this.Input.start();
> >      Output.start();
> > +    //PointerAdapter.start();
> I hope this is not an actual change you are proposing :)
> Maybe this could be exposed as a preference?

Yes, this is not an actual change. 

Just an environment setting to let the screen reader to reach this bug.

Actually the bug wouldn't happened in current case.

But I think there are still some problems in this part of code.

Thanks for the advice! expose as an preference sounds a good idea if we need to block PointerAdapter for other usage in the future :)
Assignee: nobody → kechen
Hello Eitan, can you help me to check how the highlight box should react when doing the scroll and pinch action? The situations are described in Comment 1.

It seems the highlight box's reaction before Bug 887586 is different from now. Is there any definitions to the reactions in these situations? Or these situations are undefined then?
Assignee: kechen → nobody
Flags: needinfo?(eitan)
Sorry for the late reply. I am having trouble reproducing this. If you remove the pointer adapter, how do you even set the virtual cursor? I tried to reproduce by simply not doing preventDefault and stopPropogation[1]. I could scroll and pinch web content in the browser app, and it is updated correctly.

The current architecture makes it laggy. This hasn't been an issue with the screen reader so far, because we don't allow touch zoom/pan. The devtools highlight works in a similar way, and has similar issues.

Again, I can't reproduce the issue you see.
Flags: needinfo?(eitan)
This is a demonstration of the case I mentioned. In this video I use attachment 8585852 [details] [diff] [review] to block PointerAdapter and touching the screen to set the virtual cursor.

In video the bottom part is an iframe. In the end of the video, I set the virtual cursor in the iframe first and do the pinch zoom, the virtual cursor is then set out of the iframe.

And yes this is not an issue in current screen reader, I just find this situation when I work in another bug. This situation happened because I change the act of the screen reader.

Thank you for the response :) 
So maybe I should skip this currently?
Flags: needinfo?(eitan)
I still can't reproduce this. I got caught up in a more severe bug, where explore by touch doesn't work with scaled content. Bug 1160259.

What specific app/url are you using for this test?
Flags: needinfo?(eitan)
You need to log in before you can comment on or make changes to this bug.