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

Support a11y on mobile devices #852

Closed
jessegreenberg opened this issue Aug 28, 2018 · 40 comments
Closed

Support a11y on mobile devices #852

jessegreenberg opened this issue Aug 28, 2018 · 40 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

This is a more up to date continuation of #41 which has gotten old and a bit stale. See that issue for documentation about our thoughts on the current approach for this and how it is working.

@jessegreenberg
Copy link
Contributor Author

The above commit hides this behind a query parameter. In doing so, I noticed that a lot of this work has been spread around Display, AccessibilityTree, AccessibleInstance, AccessiblePeer...and attributes added to HTMLElements throughout these types are related and dependent. This will make maintenance difficult, it would be great if we could consolidate this better.

@jessegreenberg
Copy link
Contributor Author

I just noticed that the positioning isn't quite correct for some buttons in rutherford-scattering.
capture

But if the window size changes then it corrects itself. So maybe its not getting updated frequently enough.

@jessegreenberg
Copy link
Contributor Author

The above commit fixed #852 (comment).

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Sep 13, 2018

I think the last big question is how to make sure that the PDOM does not interfere with pointer input while it is on top of the simulation, while making sure that it works correctly with a screen reader.

The most elegant way is pointer-events: none and this is generally recommended when doing online searches. This prevents the PDOM from ever receiving pointer events, and they are passed through correctly to the graphical display underneath. The browser sort of treats the elements like they are invisible and the cursor is not impacted either. However this prevents almost all PDOM elements from being touchable when VoiceOver is enabled. Individual elements are still accessible with VoiceOver, but only when discovered with the virtual cursor (using gestures and flicking left and right across the screen).

In summary, we need pointer-events: none so that the PDOM does not interfere with other scenery input. But we don't want to use pointer-events: none because it prevents the user from discovering elements by touching on them with VoiceOver. This feature is most important for visually impaired users that can see while using the screen reader.

I spent some time trying to find an alternative. I started by trying to see if I could make elements with VO touching while still using pointer-events: none. Surprisingly, I noticed that the following HTML works great:

<!DOCTYPE html>
<html>

<style>

.graphics {
  z-index: 1;
}

.backDiv {
   min-width: 100vw;
   min-height: 100vh;
   background-color: rgba( 0,0,255,0.5);
   position: absolute;
   left: 0;
   top: 0;
   z-index: 2;
}

.frontButtons {
  min-width: 14vh;
  min-height: 14vh;
  transform: rotate(0.5turn);
  pointer-events: none;
}

.pdom {
  position: relative;
  z-index: 10;
  background-color: rgba( 0,255,255,0.5);
}

.simDiv {
  -webkit-user-drag: none;
  user-select: none;
  touch-action: none;
}

</style>
  <head>
    <meta charset="UTF-8">
    <title>Page title</title>
  </head>
  <body>

    <div id="simDiv" class='simDiv'>
      <div class="pdom" id="pdom">

        <!-- we want events on this to appear receivalbe, but we want all pointer  events to reach the bottom one without ever going through this -->
        <button id='frontButton' class="frontButtons">DOM Button</button>

        <input type="button" id='secondButton' class="frontButtons">
        <label for="secondButton">Second Button</label>
      </div>

      <div class="graphics">

        <!-- we want the event to reach this without touching the top one -->
        <div id="backDiv" class="backDiv">Maybe sim SVG</div>
      </div>
    </div>
  </body>
</html>

It is critical that pointer-events: none is not on the PDOM parent container, but on the individual elements. It is also important that the html elements that are touchable be on top of the others.

This is a very simple example, and I haven't been able to get this working in the sim yet. Using Weinre with the sim, I also found that tabIndex="-1" on containers for focusable elements seems to prevent touching behavior. I also found that removing attributes position and transform on elements with pointer-events: none seems to make things touchable in the sim. This makes me think the behavior could be related to the stacking context, but I haven't been able to reproduce that in the simplified example above.
EDIT: See following comment.

We are kind of reverse engineering VO here, I can't find any specification about this at all. This is a very atypical usage of HTML. So anything we do feels like a workaround and could be fragile in the long run.

A totally different approach could be to use JavaScript somehow instead of pointer-events: none. But I can't think of a way that would work because when working with VO, PDOM elements are not receiving pointerdown events to capture or delegate. VO is doing something internally, but using pointer-events: none to determine whether something is focusable or not. The visual cursor also breaks this way. I can't imagine proceeding down this path, it would impact all of scenery input.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Sep 13, 2018

but I haven't been able to reproduce that in the simplified example above.

That is not correct, I was able to prevent this from working again by adding position: absolute and transform: translate(X, X); to the elements. Though when pointer-events: none is removed, it works just fine of course.

When I remove those attributes in the sim, everything seems touchable, but it is a little hard to tell because everything is so out of place.

@jessegreenberg
Copy link
Contributor Author

Interesting, if I use transform=static and on the elements it works OK in VO and the transformations seem to still look OK. This is even true when pointerEvents='none' is on all elements.

@jessegreenberg
Copy link
Contributor Author

In the simulation this seems to work for everything except the HomeScreenView and navigation bar!

@jessegreenberg
Copy link
Contributor Author

With the above commit it is working much better. I found that transform on non-focusable elements was for some reason preventing the touch to focus feature in mobile safari. So I changed the implementation to only transform focusable elements. This actually simplified the transformation calculations quite a lot. I am now using node.getLocalToGlobalMatrix() instead of calculating the relative transformation matrix between parent and child AccessibleInstances, and that may be less efficient. But we are doing the operations far less than we were.

I also found that for focusable elements with labels, VoiceOver will include the label bounding client rectangle in its black highlight for the focusable node. This was a problem because VoiceOver will send a fake pointer event at the center of this rectangle. So if the label is shifted off screen, the event will be at the wrong location. I tried to overlap labels and focusable elements, but then the "touch-to-focus" feature broke again because touching the element only read the label. I tried many CSS like hidden, visibility, text fonts, opacity, others, but none of them worked.

As a workaround, I am making the label element tiny, and putting it at the very bottom of the focusable element. It looks something like this:
capture

The tiny white dash below the check box is the label. This way, the label doesn't overlap with the input, but it is near enough to the input that VoiceOver will send a fake pointer event to the correct location.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Sep 20, 2018

AccessibilityTree currently has AccessibilityTree.updateCSSTransformsForSubTree( rootInstance );, and the reasoning behind it is

Once it finds this peer, it will update the CSS transforms for AccessiblePeer elements on that AccessibleInstance and all of its descendants (in that order because parents need to be updated before children due to the computation of the matrix that transforms DOM to the local node coordinate frame).

I don't think this is necessary anymore, if transform is only on focusable elements, updating will never impact the bounds of ancestors.

UPDATE: When I remove the recursive nature of updateCSSTransformsForSubTree, things no longer work correctly. Why?

UPDDATE: I had to remove this line from updateDirtyCSSTransforms:

      // no need to continue searching down this sub tree after this traversal
      rootInstance.peer.descendantTransformDirty = false;

UPDATE: This works, but I am not actually sure if it is better yet.

UPDATE: It isn't better, and the current approach of finding the subtree to update doesn't work if we remove this. We need to check the entire subtree under an AccessibleInstance marked as dirty so we can keep peer.descendantTransformDirty up to date. It was "working" before because we just updated one sibling element every animation frame. Things eventually were correct but it took many frames to get there. Hey, this could be a way to improve performance! But it feels very sloppy, lets not do that yet.

UPDATE:

But it feels very sloppy, lets not do that yet.

This is just a bad idea, what if some root level element updates every frame?

@jessegreenberg
Copy link
Contributor Author

I just noticed in pdom-transfer branch that NumberPicker is not focusable. I just merged master into the branch and the problem is still there. What changed?

@jessegreenberg
Copy link
Contributor Author

At one point it looked like tabindex played a role in what was and wasn't touchable with VO, so it was removed. Hopefully we can add it back now.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Sep 20, 2018

Hmm, yes tabIndex does seem to impact this, but I don't know why. Some buttons behave as though their touch ares are not around the highlight or VoiceOver highlight. This is not the case with <input> but it is the case with most <button>s.

Why does VO care about this? Isn't tabindex=0 the default for focusables?

This may or may not be helpful: https://stackoverflow.com/questions/18167981/clickable-link-area-unexpectedly-smaller-after-css-transform

@jessegreenberg
Copy link
Contributor Author

It was as though the touch area was around the top left corner of the element, so I thought it could be related to these lines of the CSS

        domElement.style.top = '0px';
        domElement.style.left = '0px';
        domElement.style.padding = '0px';
        domElement.style.transformOrigin = 'left top';

According to https://developer.mozilla.org/en-US/docs/Web/CSS/top

When position is set to absolute or fixed, the top property specifies the distance between the element's top edge and the top edge of its containing block. (Containing block needs to have property position: relative)

So I changed AccessibilityUtil.createElement to

        if ( focusable ) {
          domElement.style.position = 'fixed'; // helps move container elements out of the way, doesn't seem to interfere
        }
        else {
          domElement.style.position = 'relative';
        }

and the touch areas are much improved. This is too crude, and won't work for label siblings. We need to update these accordingly when focusable changes, but this is a very helpful find.

@jessegreenberg
Copy link
Contributor Author

#852 (comment) is not correct. I had accidentally left pointer-events: none in place, which I sometimes remove to easily inspect a11y items in the dev tools.

@jessegreenberg
Copy link
Contributor Author

The following button has this problem:

<button id="15-25-51-52-53-50" aria-roledescription="Sim Screen" style="transform-origin: left top 0px;  pointer-events: none; transform: matrix(2.12098, 0, 0, 13.4891, 606.111, 367.688);">‪Plum Pudding Atom‬</button>

If I remove EITHER transform OR pointer-events from the element the problem goes away. One of the resources I found said they had better luck using a matrix3d, but I have not found that to be the case. For some reason the problem goes away when I add Y scaling.

@jessegreenberg
Copy link
Contributor Author

What if we change to use width, height, and positioning instead of transform? The idea is we would calculate the right transform, then apply to the clientWidth and clientHeight (before any transforms), then use the transformed element to get the width and height from getBoundingClientRect(), then apply with width and height attributes instead of transform. Will this make Safari happy?

@jessegreenberg
Copy link
Contributor Author

Wow, this is actually working really well, this button behaves great in Safari:

 <button id="15-25-51-52-53-50" aria-roledescription="Sim Screen" style="position: fixed; pointer-events: none; width: 361.200; height: 374.535; top: 456.59; left: 259.61">‪Plum Pudding Atom‬</button>

Where the width and height were calculated from getBoundingClientRect(). Ill try it in the sim.

@jessegreenberg
Copy link
Contributor Author

Oh my goodness this is working so well!

@jessegreenberg
Copy link
Contributor Author

I added unit tests for this in the branch. I am going to run through aqua tests once more then merge with master.

@jessegreenberg
Copy link
Contributor Author

aqua tests are running well so I am going to proceed with merging. Ill continue to watch CT for the next couple of hours though.

@zepumph can we go over this issue at our next a11y dev meeting? I would like to check PDOM changes with you, but am also curious of your thoughts on the changes to AccessiblePeer. Like am I correctly adhering to model-view separation for a11y code in scenery, or should some things be moved out of AccessiblePeer/AccessibleInstance, and so on.

jessegreenberg added a commit that referenced this issue Feb 18, 2019
@jessegreenberg
Copy link
Contributor Author

The above review comments came from discussion with @zepumph, @jessegreenberg will address these review comments.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Feb 27, 2019

In phetsims/a11y-research#133 it was discovered that NVDA sends fake pointer events (in addition to the change event) upon activation of check boxes. There are probably other cases/devices that do this too.

Previously this wasn't a problem because the pointer events were well outside of the sim div. But now that elements are on top of the display they are getting caught. The result is that check boxes are activated twice, once on change and once on touch related events.

When this happens, the target of the DOM event is within the PDOM, so I am wondering if we can prevent touch events if they are sourced from PDOM elements.

EDIT: This cannot be done in Input.js as the touch event has already come hit the input element in the PDOM before bubbling up to the root.

EDIT: And to correct the above, it is the mousup event that NVDA is sending, not touchend.

EDIT: In the following JSFiddle, we get all of these events when NVDA is turned on:

touchstart _display:40:2
mousedown _display:37:2
touchend _display:43:2
mouseup _display:34:2
click _display:46:2
change

EDIT: Adding these to AccessiblePeer seems to fix the problem:

      this._primarySibling.addEventListener( 'touchstart', function( event ) {
        console.log( 'caught touch start' );
        event.preventDefault();
        event.stopPropagation();
      } );

      this._primarySibling.addEventListener( 'touchend', function( event ) {
        console.log( 'caught touch end' );
        event.preventDefault();
        event.stopPropagation();
      } );

      this._primarySibling.addEventListener( 'mousedown', function( event ) {
        console.log( 'caught mouse down' );
        event.preventDefault();
        event.stopPropagation();
      } );

      this._primarySibling.addEventListener( 'mouseup', function( event ) {
        console.log( 'caught mouse ups' );
        event.preventDefault();
        event.stopPropagation();
      } );

With the above changes, we are still receiving an error because NVDA is trying to send the click event to the label. I am not clear why. event.target is label#label-376-440-454-662-661-655.a11y-sibling

EDIT: OH, the label received the click event because I pressed enter while the virtual cursor had seleted the label element.

@jessegreenberg
Copy link
Contributor Author

#852 (comment) was fixed via #949.

@zepumph
Copy link
Member

zepumph commented Jan 14, 2020

@jessegreenberg, what more needs to be done here? Is there anything I can do to help?

@zepumph zepumph removed their assignment Jun 11, 2020
@jessegreenberg
Copy link
Contributor Author

This issue can be closed, we have been using this for a long time and it has been working well. This became sort of a mega-thread for issues related to mobile a11y. I reviewed comments in this issue and it looks like everything noted has been resolved. Other issues since then that are related to this have been opened, which is what we should continue to do in the future.

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

No branches or pull requests

2 participants