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

NUX: Make tips collapsed by default #11281

Closed
wants to merge 5 commits into from
Closed

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Oct 31, 2018

Closes #9242. Fixes #12358.

nux

Makes the NUX guide less overwhelming by collapsing the first tip in the guide.

What's changed:

  • DotTip has a new isCollapsible prop which marks it as being a collapsible tip
    • Collapsible tips are opened and closed by clicking on the indicator
    • I've increased the size of the indicator so that it's easier to click
    • The first tip is marked as isCollapsible
  • Dismissing and disabling tips works as before

How to test

  1. Clear out your local storage or re-enable tips
  2. Note that the first tip is now a collapsed pulsating dot
  3. Click on it to open up the first tip
  4. Click through the rest of the tips

@noisysocks noisysocks added [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. [Feature] NUX Anything that impacts the new user experience labels Oct 31, 2018
@karmatosed
Copy link
Member

Thanks for this. I do agree that having the pulsing is much better. Unfortunately in testing though there are a few issue that to me mean we have to iterate:

1

Ideally we would I think only have in a walkthrough as the Tips are today, one showing at a time. This multiple pulsing being the first experience is too much.

2

Whilst this goes when we only have one by one, having one Tip open is a weird experience right now. For now having it just become one by one works. Pulsing one opens to show Tip and then when you close a new one pulses.

3

As illustrated above, having multiple Tips open is easy with them all showing. This isn't a great experience so let's step back to just 1 by 1 in a flow.

That all said, I do think the pulsing itself is much better. My issue is with the number :)

Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

As per my comment, let's have only one at a time for now.

@noisysocks
Copy link
Member Author

noisysocks commented Nov 13, 2018

@karmatosed: I've made it so that only the first tip is collapsed! Give me a 🙂 or a 🙁 before I start work on making the tests pass so that we can merge this.

@karmatosed
Copy link
Member

Thanks, that's a lot better!

@noisysocks noisysocks removed the Needs Design Feedback Needs general design feedback. label Nov 16, 2018
@noisysocks noisysocks removed the [Status] In Progress Tracking issues with work in progress label Nov 16, 2018
@noisysocks
Copy link
Member Author

Thanks @karmatosed! ❤️

I've made the tests pass, updated the PR description, and tagged some folks for a code review.

@noisysocks noisysocks added this to the 4.5 milestone Nov 16, 2018
@tofumatt tofumatt added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 16, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This seems like quite an aggressive change to be making well after a UI freeze and so close to the 5.0 release.

There are probably accessibility concerns with closing these by default. What's the intended UX for screenreaders, for instance? Do we want the tips to need to be "activated" by screenreader users (I don't think that's ideal/good UX) or should they be active by default in the DOM/screenreader UX and just visually hidden (I think that's better)?

I'll need to do a fair amount of testing on this, later in the day.

@tofumatt tofumatt self-requested a review November 16, 2018 14:52
@karmatosed
Copy link
Member

@tofumatt having multiple tips and having them open by default also has accessible issues. The goal here is to make it more usable. What can be done with it closed to pass a11y?

@tofumatt
Copy link
Member

If there will be multiple tips present in the UI with only a visual indicator of its relation to the UI, a screenreader won't be able to contextualise it. So each tip should have a label/short descriptor/intro to what the tip is about so a screenreader user can choose to expand it. That would be a similar UX for a screenreader and allow the tip to be hidden by default (and also allow a few on-screen at the same time without confusion).

If we can't do that timing-wise... it should probably be the case that screenreader users don't have to "Open" the tips as that's a visual UX—the tip is visually hidden to reduce visual clutter, but for a screenreader it can't relate the tip to the visual UI. Best to just read it out.

Does that make sense?

@gziolo gziolo added [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Priority] High Used to indicate top priority items that need quick attention Needs Decision Needs a decision to be actionable or relevant and removed [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later labels Nov 19, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Maybe I'm missing something, but how are these tooltips announced to users of assistive technology? I don't see them being announced and I can't see how to access them with the keyboard when they're hidden. Additionally: when I clicked on one the screenreader didn't read out the contents.

These seem like an accessibility regression, which is a real shame because these tips would be useful for explaining things like keyboard shortcuts to users.

When I open a page with tips on master, the tips are read out once I tab at least... it's not great but it works. On this branch I can't tab to the tips obviously and they're basically lost on a keyboard/screenreader user.

I would think that the best way forward would be to have tips use wp.a11y.speak( $short_tip_description ) to announce themselves to screenreader users and provide an obvious way to access them with a keyboard. Maybe include a keyboard shortcut to open them in the short description.

@karmatosed
Copy link
Member

@tofumatt can they announce and be closed? Just want to work out boundaries here.

@tofumatt
Copy link
Member

tofumatt commented Nov 19, 2018 via email

@mtias
Copy link
Member

mtias commented Nov 19, 2018

This seems like it needs a bit more work to cover all the cases.

@noisysocks
Copy link
Member Author

Picking this back up. It's something that I think we should do as part of 5.0.1, which means that there's some time to iterate on a good solution here.

The button which opens a collapsed tip appears in the DOM like any other button. When I enable VoiceOver, check this branch out, and then tab through the toolbar, I can hear VoiceOver read:

  • "Add block, selected, button"
  • "Open tip, selected, button"
  • "Undo, selected, button"

Would changing the label of the Open tip button to something more explanatory suffice? e.g. "Open tip for 'Add block'"

I get your point about it not being immediately obvious to users of screen readers that there are tips visible on the screen. I'll need your guidance here as I'm not familiar with wp.a11y.speak() best practices. Would speak()ing something like "There is one tip on the screen. Press ^⌥t to change focus to the next tip" when the page loads work?

Stepping back, I'm not clear on how best to make the NUX guide useful to users of screen readers since much of it is premised on visually pointing to UI elements. I could use your guidance there, too.

@karmatosed
Copy link
Member

I agree @noisysocks. I do think it's not great they are how they are right now but it is something that can be iterated.

@tofumatt
Copy link
Member

"Open tip for 'Add block'"

works for me. 👍

Announcing there are tips available on page load might not be great actually after thinking on it because speak() is largely used to convey information changed after interaction that a screenreader won't otherwise pick up on.

@noisysocks noisysocks changed the title NUX: Try making tips collapsed by default NUX: Make tips collapsed by default Nov 28, 2018
@noisysocks noisysocks removed the Needs Decision Needs a decision to be actionable or relevant label Nov 28, 2018
@noisysocks
Copy link
Member Author

I've updated this to use a more descriptive label for screen readers.

@tofumatt
Copy link
Member

Thanks! 👍

(Feel free to flag me for another review once you'd like me to have another look—even if things aren't finished yet.)

@noisysocks
Copy link
Member Author

I think this is ready to go in. Just needs a code review. No rush though since it's targeted for 5.0.1 🙂

@tofumatt tofumatt self-requested a review November 28, 2018 01:43
@mtias mtias modified the milestones: 4.7, 4.8 Nov 29, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

The issue with this is there still isn't an announcement of the fact that there are tips anymore with them hidden. There's no awareness that a tip exists (or how to activate it) for a screenreader user.

In master the tip, in its entirely, is read aloud to the user. Here's NVDA's speech log with it in action:

screenshot 2018-12-03 20 20 47

In this branch there's nothing like it so there's just no context for a tip existing.

What we need is for there to be HTML in the DOM that is read out by a screenreader instructing the user that there is a tip available and how to access it. The old tip's content were read out by the screenreader... if that didn't work then wherever the code that checks if tips should be displayed should probably use speak() to say something like speak( 'Tips on new features are available; press $KEYBOARD_SHORTCUT to open them.' ) or something like that.

<DotTip
tipId="core/editor.inserter"
isCollapsible
label={ ( isOpen ) => isOpen ? __( 'Close tip for ‘Add block’' ) : __( 'Open tip for ‘Add block’' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Because we use US English as our default language we should use double-quotes here, not single. Single-quotes are a British English thing 😅

@afercia
Copy link
Contributor

afercia commented Dec 4, 2018

I see only the first tip is a button. All the other ones are divs, so their popover can't be closed and have the pulsing tip still active. At that point, users have either to go through all the tips or dismiss them permanently.

Basically this proposed change collapses only the first tip. Not sure what the added value is other than saving a few clicks. Seems to me this change tries to solve a discoverability problem by means of a more complicated and less intuitive interaction with the tips.

That said, re: the first tip (the button one): while it has an aria-label, it also needs to expose its name. This was mentioned several times on other issues. For example, how a speech input users is supposed to activate the first button? The accessible name (the aria-label) of the button is not exposed visually in any way:

screenshot 2018-12-04 at 18 05 18

For normal buttons, at least we have tooltips (which are already a big trade-off):

screenshot 2018-12-04 at 18 05 29

Pre-existing issues:

  • the animation on the pulsing tip makes the CPU fan go crazy after a couple minutes, even on a MacBook Pro. Wondering what happens on less performant hardware. As this change allows users to keep the first tip active and pulsing indefinitely in the UI, it's very likely we're going to stress their CPU
  • in some steps, the tooltip of the "X" close button is still hidden behind the popover:

screenshot 2018-12-04 at 18 21 19

- Adds `isCollapsible` to `DotTip` so as to support collapsible tips.
  These are tips that only show the indicator dot. When the indicator dot
  is clicked, the tip opens like normal.
- Make the NUX guide less overwhelming by collapsing the first tip in
  the guide.
@noisysocks
Copy link
Member Author

noisysocks commented Dec 6, 2018

Thanks for the feedback everyone!

What we need is for there to be HTML in the DOM that is read out by a screenreader instructing the user that there is a tip available and how to access it.

I've had a first pass at doing this in 1874e0e. Let me know what you think and we can iterate on the approach 🙂

Basically this proposed change collapses only the first tip. Not sure what the added value is other than saving a few clicks.

cc. @karmatosed who can explain the motivation here. My understanding is that we want to make a user's first impression of the new editor less overwhelming by not having the first tip appear so intrusively on the screen.

That said, re: the first tip (the button one): while it has an aria-label, it also needs to expose its name.

Good catch—I've added a tooltip in e06f982.

Pre-existing issues

Are there issues for these? They sound like bugs that we should fix regardless of whether this PR lands.

Attempts to improve the accessibility of tips by:

- Allowing `DotTip` to be expanded or collapsed using a keyboard
  shortcut configured with the `shortcut` prop.
- Having collapsed tips that have a shortcut configured `speak()` when
  mounted so that screen reader users are aware of their presense.
@afercia
Copy link
Contributor

afercia commented Dec 6, 2018

Are there issues for these? They sound like bugs that we should fix regardless of whether this PR lands.

  • first one (tip animation / CPU load): I don't think there's an issue for that. Maybe it was mentioned in another issue but I'd like to be confirmed before creating an issue,. Would be great to test on a medium / cheap hardware
  • second one (tooltips hidden behind popover): NUX tips: Close button tooltip partially hidden on some tips #7562 created on June

make a user's first impression of the new editor less overwhelming by not having the first tip appear so intrusively on the screen.

I understand the goal of this PR 🙂 I'm just saying that it makes things more complicated, interaction becomes unexpected, at the point I wonder if tips still honor the reason they were implemented for. The original goal was: on first load you see a message that encourages you to discover more about the new experience. Now instead we want to hide them ¯_(ツ)_/¯

@mtias mtias added Needs Design Feedback Needs general design feedback. and removed [Priority] High Used to indicate top priority items that need quick attention labels Dec 6, 2018
@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@noisysocks
Copy link
Member Author

After speaking with @karmatosed, we're shelving this PR for now. In the future we would like to revisit the UX of tips more whole holistically.

@noisysocks noisysocks closed this Jan 7, 2019
@aduth aduth deleted the try/collapsible-tips branch January 25, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] NUX Anything that impacts the new user experience [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First welcome tip obscures the UI element that it references Scale back tips to just be pulse
7 participants