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

Can't open my challenge accordion box by clicking on the title with ?shuffleListeners #328

Closed
zepumph opened this issue Dec 29, 2020 · 5 comments
Assignees
Labels
type:bug Something isn't working

Comments

@zepumph
Copy link
Member

zepumph commented Dec 29, 2020

I think this has to do with the refactor in 5d3cd2a for #286. I may have also run into the before, but I forget when or where.

@zepumph zepumph self-assigned this Dec 29, 2020
@zepumph zepumph added dev:code-review type:bug Something isn't working and removed dev:code-review labels Dec 29, 2020
@zepumph
Copy link
Member Author

zepumph commented Dec 29, 2020

Ahh, I remember, this was an issue in #88.

@zepumph
Copy link
Member Author

zepumph commented Dec 29, 2020

UPDATE: this only occurs when ?shuffleListeners is present.

@zepumph zepumph changed the title Can't open my challenge accordion box by clicking on the title Can't open my challenge accordion box by clicking on the title with ?shuffleListeners Dec 29, 2020
@zepumph
Copy link
Member Author

zepumph commented Dec 29, 2020

Ok, I cannot consistently get this to reproduce. I believe I have only been able to reproduce this with both ?shuffleListeners&log, but not every runtime.

http://localhost:8080/ratio-and-proportion/ratio-and-proportion_en.html?brand=phet&ea&shuffleListeners&log would do it about 40% of the time. Otherwise you could click the collapsed title bar just fine.

@zepumph
Copy link
Member Author

zepumph commented Dec 30, 2020

Ok. I think I have cracked it. It only happens some times because it depends on the random listener order of the pickableProperty listeners. Since Node got updated to use TinyProperty, it was rewritten to add a prioritized listener first, before any others. If this get's jumbled, then I would expect for scene graph operations (like the hit test for the accordion box to) to be buggy. I'm going to create a new issue to handle this so that ?shuffleListeners is still valuable. to the project.

@zepumph zepumph closed this as completed Dec 30, 2020
@zepumph
Copy link
Member Author

zepumph commented Dec 30, 2020

Note that this patch also fixes the problem, though we don't want to go down this road:

Index: js/AccordionBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/AccordionBox.js b/js/AccordionBox.js
--- a/js/AccordionBox.js	(revision fe13dfe1ea3f3ae0721fa5ec1100cce08efa1464)
+++ b/js/AccordionBox.js	(date 1609284982743)
@@ -163,6 +163,7 @@
       this.titleNode = new Text( '', { tandem: options.tandem.createTandem( 'titleNode' ) } );
       this.disposeEmitterAccordionBox.addListener( () => this.titleNode.dispose() );
     }
+    this.titleNode.pickable = false; // we want the pointer event to go to the Title bars, which have listeners
 
     // @public {Property.<boolean>}
     this.expandedProperty = options.expandedProperty;
@@ -270,7 +271,8 @@
       const outlineOptions = {
         stroke: options.stroke,
         lineWidth: options.lineWidth,
-        cornerRadius: options.cornerRadius
+        cornerRadius: options.cornerRadius,
+        pickable: false
       };
 
       // @private

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant