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

Reentry detected in Edge #92

Closed
jessegreenberg opened this issue Nov 28, 2018 · 10 comments
Closed

Reentry detected in Edge #92

jessegreenberg opened this issue Nov 28, 2018 · 10 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

For some reason, I am seeing reentry errors in master, but so far only in edge. Happens when updating the range of the objects.

My guess right now is that the flow is like

  • Position changes
    • Force changes
      • enabled range changes
        • forces position to change?

Assigning to @mbarlow12 because he is responsible dev, but also myself since I have easy access to Edge.

@jessegreenberg
Copy link
Contributor Author

I see in the call stack that we are handling both input AND keydown in edge. I would have expected event.preventDefault(); to prevent this.

@jessegreenberg
Copy link
Contributor Author

This is also definitely the cause of #90

@jessegreenberg
Copy link
Contributor Author

It appears that preventDefault isn't working because input event isn't cancelable (like the change event). So we need to do something else. This isn't a problem because Chrome doesn't trigger the input event on sliders.

@jessegreenberg
Copy link
Contributor Author

@mbarlow12 I verified that this doesn't impact the coulombs-law RC because handleInput was added to AccessibleSlider after SHAS for 1.0 were taken.

@arouinfar
Copy link
Contributor

While not strictly for phetsims/qa#221, I'm tagging it because this'll need to be addressed before publication.

@jessegreenberg
Copy link
Contributor Author

Setting the 'min' attribute is causing the input event to trigger.
self.setAccessibleAttribute( 'min', enabledRange.min );,

And so is blur (sometimes).

@jessegreenberg
Copy link
Contributor Author

One potential fix is to block input events when changing the min and max attributes. After fuzz testing and manual testing I haven't been able to reproduce the bug. But this didn't fix #90 and I still think they may be related so Ill investigate that issue before commiting this.

@jessegreenberg
Copy link
Contributor Author

If I use "any" as the value for the step attribute, this problem goes away, see phetsims/sun#413

@jessegreenberg
Copy link
Contributor Author

Input event is still triggered sometimes when range of slider updates.

The flow of reentry is

  • position changes
    • force changes
      • enabled range changes
        • triggers an input event
          • input listener triggers position change

@jessegreenberg
Copy link
Contributor Author

The above commit blocks input events after capturing a keydown event, which is OK since keydown handles all keyboard logic and input is purely there for assistive devices without keyboards. This has been fixed in master, and doesn't need to be included in the release since RC shas of sun with AccessibleSlider don't include handleInput. Closing.

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

No branches or pull requests

3 participants