Skip to content

Conversation

jart
Copy link
Contributor

@jart jart commented Aug 25, 2017

#261 recently removed the expand button from the image dashboard, which freed up whitespace. However the checkbox in the top left isn't conspicuous enough. With this change, clicking on an individual image, will toggle its actual size state, for only that image. This seems intuitive to me.

@jart jart requested a review from chihuahua August 25, 2017 22:22
@chihuahua
Copy link
Member

This seems great. Want to add pointer: cursor; to the container to indicate to the user that the image is clickable?

@jart
Copy link
Contributor Author

jart commented Aug 25, 2017

Cursor pointer now used. Although I'm not 100% sure we want to do that without supporting auxiliary click, which Polymer makes difficult to do.

@chihuahua
Copy link
Member

OK, sounds good.

@wchargin
Copy link
Contributor

This is not accessible.

The best solution is to use an <button> for buttons, as opposed to a <div> with listeners. If you can find a way to do that, that would be great: all browsers and assistive tools will do the right thing without any interaction management. An <a>nchor is also an acceptable solution, though <button> is better. (Note that you can style buttons just like anything else, so they don't have to have that "traditional button look": on this page, for instance, note that "No one—assign yourself", the "status okay" checkmark next to the commit SHA, and "Show all reviewers" are all <button>s.)

If for some reason there is no way to get the above to work, then at the very least you'll need to add tabindex="0" to the <div id="main-image-container"> so that it's keyboard-focusable. I'm not sure what Polymer's on-tap does; you'll probably need to add an onkeypress listener that toggles the actual size when Enter and Space are pressed, as well. You will also need to add role="button" to the div. This approach is much less good because it requires emulation of potentially platform-specific functionality (for instance, I use an extension that lets me interact with mouse elements using the keyboard, and I'm not sure that it would work in this case; actual accessibility tools will face similar problems).

@chihuahua
Copy link
Member

Making TensorBoard accessible seems like a worthy pursuit - I'm sure there are ML practitioners who are blind for instance.

The harder question seems like how. How do blind people conduct exploratory data analysis? I wonder if they rely on different hardware with say haptic or auditory affordances. How is a line chart, histogram, or image/heat map presented to a blind person?

It might be interesting for someone to sit down with a blind researcher and observe how they operate. And if it turns out that few blind researchers conduct EDA, maybe it's an opportunity and challenge for TensorBoard to bring more blind people into the fold of model interpretability.

In terms of short-term actionable items, I like William's thoughts about following web development norms like using tabindex and aria-label for now, so that blind users can at least navigate TensorBoard and perhaps enter the text dashboard.

@jart
Copy link
Contributor Author

jart commented Aug 27, 2017

Thank you William for reminding us of the importance of good Internet citizenship. The code has been updated. There's more we can be doing regarding i18n, l10n, and a11y. So I hope we can find more low hanging fruit opportunities to improve things on this front in the near future, like we did this summer regarding color blindness.

@jart jart merged commit b1a4d25 into tensorflow:master Aug 27, 2017
@jart jart deleted the image-scaling branch August 27, 2017 21:17
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: this is keyboard-accessible for me in Chrome and Firefox, and the focus states seem sufficiently visible. This also passes tota11y inspection. Thanks for fixing!

The cursor: pointer doesn't seem to work in Firefox, but that's not a big deal to me.

I'm assuming that it's intended behavior that if you (1) toggle an image's local actual size, then (2) toggle the dashboard-level actual size twice, then the effect of (1) is reverted. I have no problem with this.

(Note to self…if you submit a review with the "Approve" option, but the pull request submitter merges the request between the time that you load the page and the time that you submit the review, then GitHub says "Can only approve open pull requests" and permanently discards your message. Shame.)

jart added a commit that referenced this pull request Aug 30, 2017
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