Make collapse diff buttons keyboard accessible

Review Request #10925 — Created Feb. 27, 2020 and updated

Information

Review Board
master
8679dc4...

Reviewers

Previously, the collapse diff button on reviewable diffs and diffs in
comment fragments could not be used with the keyboard as shown in the
attached video collapse-btn-old.mov, unlike the expand diff button. This
review request makes the collapse diff button keyboard accessible. The new
behavior is documented in the attached video collapse-btn-new.mov. There
is also a side-by-side screenshot comparison of the old and new button.

The collapse button used to be a <div>, which is not focusable by
default. The fix is to change the <div> to an <a>, which made the button
focusable but did not invoke the collapse diff action. (Using an <a> is
consistent with the expand diff button.) By adding the href=# attribute,
both clicking on the element and using the enter key while the element is
focused invokes the click handler to collapse the diff.

Manual testing on reviewable diffs and comment fragment diffs:
- used tab & shift+tab to check the collapse diff button can be tabbed to
- used enter key to invoke the collapse diff action and re-expand diff
using only the keyboard


Description From Last Updated

A few notes on the change description: The text is wrapping far too early. Can you wrap at ~75 characters? …

chipx86chipx86

Can you show a side-by-side screenshot of an old collapse button and a new one? There's some fundamental changes to …

chipx86chipx86

The attributes aren't aligned correctly.

chipx86chipx86

The attributes aren't aligned correctly. Since you're updating this line anyway to wrap, let's move the data-... attributes to the …

chipx86chipx86
chipx86
  1. 
      
  2. Show all issues

    A few notes on the change description:

    • The text is wrapping far too early. Can you wrap at ~75 characters?
    • HTML tags should become string literals (backticks)
    • The Asana link isn't going to be useful to outside contributors, and this change description will be used when landing. You can just remove that from here.
  3. Show all issues

    Can you show a side-by-side screenshot of an old collapse button and a new one? There's some fundamental changes to the display type that's been brought in through this, and I want to make sure it's going to be consistent.

  4. Show all issues

    The attributes aren't aligned correctly.

  5. Show all issues

    The attributes aren't aligned correctly.

    Since you're updating this line anyway to wrap, let's move the data-... attributes to the second line, keep things under our line length limits.

    And fix our variable references to be {{comment.id}} instead of {{ comment.id }} (the latter must have snuck in).

    These comments apply to both modified lines in the file.

  6. 
      
hxqlin
Review request changed
Change Summary:

Fix issues from review

Description:
~  

Previously, the collapse diff button on reviewable diffs and

~   diffs in comment fragments could not be used with the keyboard
~   as shown in the attached video collapse-btn-old.mov, unlike
~   the expand diff button. This review request makes the collapse
~   diff button keyboard accessible. The new behavior is documented
~   in the attached video collapse-btn-new.mov.

  ~

Previously, the collapse diff button on reviewable diffs and diffs in

  ~ comment fragments could not be used with the keyboard as shown in the
  ~ attached video collapse-btn-old.mov, unlike the expand diff button. This
  ~ review request makes the collapse diff button keyboard accessible. The new
  ~ behavior is documented in the attached video collapse-btn-new.mov. There
  ~ is also a side-by-side screenshot comparison of the old and new button.

   
~  

The collapse button used to be a <div>, which is not focusable

~   by default. The fix is to change the <div> to an <a>, which made
~   the button focusable but did not invoke the collapse diff action.
~   (Using an <a> is consistent with the expand diff button.) By
~   adding the href=# attribute, both clicking on the element and
~   using the enter key while the element is focused invokes the
  ~

The collapse button used to be a <div>, which is not focusable by

  ~ default. The fix is to change the <div> to an <a>, which made the button
  ~ focusable but did not invoke the collapse diff action. (Using an <a> is
  ~ consistent with the expand diff button.) By adding the href=# attribute,
  ~ both clicking on the element and using the enter key while the element is
  ~ focused invokes the click handler to collapse the diff.

-   click handler to collapse the diff.

-  
-  

Asana link: https://app.asana.com/0/40883733502513/1157098340882771/f

Testing Done:
   

Manual testing on reviewable diffs and comment fragment diffs:

~   - used tab & shift+tab to check the collapse diff button can be
~   tabbed to
~   - used enter key to invoke the collapse diff action and re-expand
  ~ - used tab & shift+tab to check the collapse diff button can be tabbed to
  ~ - used enter key to invoke the collapse diff action and re-expand diff
  ~ using only the keyboard

-   diff using only the keyboard

Commit:
1c430fa229dd65539005593b50cd3a8ae087b4b4
8679dc402cabef2f347d59d2fc2afc49360e024c
Added Files:

Checks run (2 succeeded)

flake8 passed.
JSHint passed.