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

fix: throttle clickable component #8450

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Essk
Copy link
Contributor

@Essk Essk commented Sep 28, 2023

Description

See #8393

We need to throttle click events on "toggle" UI components or interactions from screen readers in the firefox browser will toggle the control on and off instantly, effectively causing the control to do nothing.

Specific Changes proposed

These changes add the ability to specify a throttle option to any component that extends ClickableComponent. The value can be true (which will set a default wait time of 50ms on the throttle) or a number >= 1, which will be passed as a custom wait time in ms.

Toggle components in the standard UI have been updated to have the option { toggle: true}.

Further thoughts

Given that the need for a throttle is governed by external factors that we cannot predict, I think it may be beneficial to throttle all clickable UI by default with an option to turn throttling off. However in practice, as things stand, this breaks a lot of videojs tests which were never written with throttled components in mind. Those internal tests could be updated* but users that have implemented custom components with their own testing are likely to suddenly find their tests failing and I think this would be considered a breaking change.

  • Updating the tests is not straightforward because as the current design of imports from utils/fn prevents stubbing the throttle utility that is imported into component classes.

Perhaps these considerations could be taken into account for VJS 9?

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
      • Will verify when I next have access to a Windows environment
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@Essk Essk changed the title 8393 throttle clickable commponent fix: 8393 throttle clickable commponent Sep 28, 2023
@Essk Essk changed the title fix: 8393 throttle clickable commponent fix: throttle clickable commponent Sep 28, 2023
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.01%. Comparing base (473176f) to head (c4aedef).
Report is 124 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8450      +/-   ##
==========================================
+ Coverage   82.71%   83.01%   +0.30%     
==========================================
  Files         113      113              
  Lines        7589     7812     +223     
  Branches     1826     1875      +49     
==========================================
+ Hits         6277     6485     +208     
- Misses       1312     1327      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return this.handleClick.bind(this);
};

const selectedClickHander = selectClickHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit sp:

Suggested change
const selectedClickHander = selectClickHandler();
const selectedClickHandler = selectClickHandler();

@@ -41,9 +46,20 @@ class ClickableComponent extends Component {
this.controlText(this.options_.controlText);
}

const selectClickHandler = () => {
if (typeof this.options_.throttle === 'number' || this.options_.throttle === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You could create a variable and reuse it here, something like:

const isNumber = typeof this.options_.throttle === 'number';

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Some minor points.

@@ -41,9 +46,20 @@ class ClickableComponent extends Component {
this.controlText(this.options_.controlText);
}

const selectClickHandler = () => {
if (typeof this.options_.throttle === 'number' || this.options_.throttle === true) {
const wait = typeof this.options_.throttle === 'number' ? parseInt(this.options_.throttle, 10) : 50;
Copy link
Member

Choose a reason for hiding this comment

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

Also, should this be parseFloat instead of parseInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured since throttle's key condition is (now - last >= wait) where now - last will always be a whole number of milliseconds, parseInt was most appropriate.

Looking with fresh eyes, maybe the parseX(arg-that-might-be-a-number) should be removed entirely as I really just put it in out of habit and I see throttle itself doesn't enforce anything.

this.handleMouseOver_ = (e) => this.handleMouseOver(e);
this.handleMouseOut_ = (e) => this.handleMouseOut(e);
this.handleClick_ = (e) => this.handleClick(e);
this.handleClick_ = (e) => selectedClickHander(e);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.handleClick_ = (e) => selectedClickHander(e);
this.handleClick_ = (e) => selectedClickHandler(e);

@@ -32,6 +33,10 @@ class ClickableComponent extends Component {
* @param {string} [options.className]
* A class or space separated list of classes to add the component
*
* @param {number | boolean} [options.throttle]
* A throttle will be applied to the clickHander if the number is >= 1 or the value is `true`
* A number specifies the desired wait time in ms or a default wait of 50ms will be applied
Copy link
Member

Choose a reason for hiding this comment

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

any details on how 50 was chosen as the default?

Copy link
Contributor Author

@Essk Essk Sep 29, 2023

Choose a reason for hiding this comment

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

Nothing empirical, just a guess on my part based on previous similar solutions I've worked on. However, this is a good point so I measured the gap on my fairly modern Windows PC, and got 1-2 ms. Sadly FF doesn't have the same performance spoofing options as Chrome (which doesn't receive double input) but maybe I can get more data from Browserstack or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad - I had hoped Browserstack might have options to set hardware limitations, but it does not.

this.handleMouseOver_ = (e) => this.handleMouseOver(e);
this.handleMouseOut_ = (e) => this.handleMouseOut(e);
this.handleClick_ = (e) => this.handleClick(e);
Copy link
Member

Choose a reason for hiding this comment

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

since we still want a bound handleClick inside of selectedClickHandler, should we keep this around somewhere and use that rather than rebinding inside selectedClickHandler?

- fix typos
- remove `parseInt`
- DRY
@Essk Essk changed the title fix: throttle clickable commponent fix: throttle clickable component Sep 29, 2023
@Essk
Copy link
Contributor Author

Essk commented Sep 29, 2023

Thanks everyone for the feedback - sorry about all the typos! 😳
I'm going to pull this back to a draft because while I've now verified the fix works with ClickableComponent derivatives in Firefox, it looks like the problem also affects MenuButton which is not a ClickableComponent so I'm thinking a throttled mixin may be the way to go instead.

@Essk Essk marked this pull request as draft September 29, 2023 11:27
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.

4 participants