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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with keyboard input in click tooltips #18044

Merged
merged 6 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@annthurium
Contributor

annthurium commented Sep 13, 2018

Description of the Change

In https://github.com/atom/atom/pull/17651/files, @cacheflow (hi! I think we follow each other on twitter, fancy seeing you here 馃槃 ) fixed a bug where tooltips were obscuring some of the areas in a user workspace, by making the tooltips disappear on keydown.

Unfortunately, this fix led to some unfortunate side effects. Some tooltips (such as Teletype, and the github atom branch creation menu) need keyboard functionality. These tooltips were closing on keypress, so users couldn't actually enter the input. Also, window.addEventListener was being called every time a new tooltip was registered/added to the TooltipManager, which led to the creation of many additional keyboard listeners. Atom generally follows the event delegation pattern, to avoid unnecessary listeners as it's bad for performance.

This fix scopes @cacheflow's original changes to only tooltips that are triggered on hover. The assumption being, hover tooltips are most likely to get in the way. The thinking is, click-triggered tooltips are more likely to be intentionally triggered (and all the input tooltips I'm aware of are click-triggered.) It also attaches/removes the event listeners when a hover tooltip is shown, rather than when the tooltip is registered with the event manager. That's going to cut down on the number of listeners.

Alternate Designs

I'm a little afraid of the event handling code in atom/atom and tbh didn't really consider any alternatives.

Why Should This Be In Core?

It's a bugfix of existing core functionality.

Benefits

Teletype and the github atom branch selector work again.

Possible Drawbacks

The handling for events in Atom is complicated. It's possible that this fix could trigger undesirable behavior. For example, if there are tooltips that are triggered by a click, that are blocking somewhere where a user wants to type, the user will have to dismiss these tooltips with a click.

Verification Process

  • Manual testing:
    • tested that typing dismisses a hover-triggered tooltip
    • tested that typing does not dismiss a click-triggered tooltip
    • tested that when a hover-triggered tooltip and a click-triggered tooltip are both showing, typing dismisses the hover-triggered tooltip but not the click-triggered tooltip
  • Unit tests
    • show a click-triggered tooltip. Fire a keyboard event. Assert the click-triggered tooltip is still there.
    • show a hover-triggered tooltip. Fire a keyboard event. Assert the tooltip has gone to the great dev/null in the sky. (Mostly I moved @cacheflow's existing test, to live with the other tests of hover-triggered tooltips, just to keep the test suite organized.)
  • negated assertions to verify that things fail as expected

Applicable Issues

#18041

annthurium and others added some commits Sep 12, 2018

fixes bug with keyboard input in click tooltips
Co-Authored-By: David Wilson <daviwil@users.noreply.github.com>
@daviwil

馃挜 馃檶

@cacheflow

This comment has been minimized.

Show comment
Hide comment
@cacheflow

cacheflow Sep 13, 2018

Contributor

I like this 馃敟. Also, should the same be done for resizing events as well? From
my understanding of the source, every time a tooltip is resized, then an eventListener is attached.

Contributor

cacheflow commented Sep 13, 2018

I like this 馃敟. Also, should the same be done for resizing events as well? From
my understanding of the source, every time a tooltip is resized, then an eventListener is attached.

@annthurium

This comment has been minimized.

Show comment
Hide comment
@annthurium

annthurium Sep 13, 2018

Contributor

@cacheflow yes, it would probably be better to fix the resizing events as well. In case there are side effects, it would probably be good to clean that up separately. I added a comment though, just so people don't add more listeners in tooltipManager in the meantime.

Contributor

annthurium commented Sep 13, 2018

@cacheflow yes, it would probably be better to fix the resizing events as well. In case there are side effects, it would probably be good to clean that up separately. I added a comment though, just so people don't add more listeners in tooltipManager in the meantime.

@cacheflow

This comment has been minimized.

Show comment
Hide comment
@cacheflow

cacheflow Sep 13, 2018

Contributor

I'll tackle that in another commit. @annthurium.

Contributor

cacheflow commented Sep 13, 2018

I'll tackle that in another commit. @annthurium.

annthurium added some commits Sep 14, 2018

@annthurium annthurium merged commit f986dfd into master Sep 14, 2018

3 checks passed

Atom Pull Requests #20180914.11 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-sept-tooltip-bug branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment