Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add onClick to Markdown buttons #2677

Merged
merged 2 commits into from
Feb 22, 2019
Merged

Add onClick to Markdown buttons #2677

merged 2 commits into from
Feb 22, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Feb 21, 2019

The Markdown toggles need to watch mousedown to properly update state, so
let's drop back down to regular divs.

This also fixes element-hq/element-web#8866.

@jryans jryans requested a review from a team February 21, 2019 18:52
@bwindels
Copy link
Contributor

hmm, should we manually add the aria shenanigans to keep this accessible?

@jryans
Copy link
Collaborator Author

jryans commented Feb 22, 2019

hmm, should we manually add the aria shenanigans to keep this accessible?

Hmm... I guess alternately, I could still use AccessibleButton, but then pass a no-op click handler.

@bwindels
Copy link
Contributor

hmm, should we manually add the aria shenanigans to keep this accessible?

Hmm... I guess alternately, I could still use AccessibleButton, but then pass a no-op click handler.

Or that, yeah. Did you figure out why onClick wouldn't work in the end? I tried, and it ended up being a weird tri-state of enabled | nothing | disabled ...

@jryans
Copy link
Collaborator Author

jryans commented Feb 22, 2019

Did you figure out why onClick wouldn't work in the end? I tried, and it ended up being a weird tri-state of enabled | nothing | disabled ...

Right. After some more inspection and hair-pulling, there seems to be a race when toggling Markdown sets state in MessageComposerInput, but then it passes the old value up to the parent MessageComposer. The step to notify the parent is embedded in the very general onChange function for all input state changes, so many different operations can cause it to fire. Since mousedown happens before click events, it seems it would win the race.

My current plan is to go with onClick again and also avoid this fun race.

@jryans
Change Markdown buttons to onClick
3e6199c 
`AccessibleButton` expects click handlers, which we can use here, as long we
make some additional changes to avoid race processing the new state (see
subsequent patch).

Fixes element-hq/element-web#8866
@jryans
Isolate rich text notifications from other input changes
ccd4dee 
The step that would notify parent components of rich text state changes was
stirred into the input's change handler, which leads to race which the parent is
sometimes notified of the old rich text state instead of the new.

Here we avoid this complication by using a separate path for sending the rich
text state when we know we have updated it correctly.
@jryans jryans changed the title Use regular divs for Markdown buttons Add onClick to Markdown buttons Feb 22, 2019
@jryans jryans assigned turt2live and unassigned jryans Feb 22, 2019
turt2live
turt2live approved these changes Feb 22, 2019
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Looks much better than before - thanks!

@turt2live turt2live assigned jryans and unassigned turt2live Feb 22, 2019
@jryans jryans merged commit c45ca58 into matrix-org:develop Feb 22, 2019
@jryans jryans mentioned this pull request Feb 22, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@turt2live turt2live turt2live approved these changes

Assignees

@jryans jryans

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A button in the composer is missing an onClick
3 participants
@jryans @bwindels @turt2live