UP/DOWN keys don't work to navigate the call tree in the new performance tool

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: vporof, Assigned: jsantell)

Tracking

unspecified
Firefox 38
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Blocks: 1075567
(Reporter)

Comment 1

4 years ago
This is more annoying that beetlejuice, damn it.
(Reporter)

Updated

4 years ago
Blocks: 1123815
Created attachment 8560692 [details] [diff] [review]
1111020-fix-key-events.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c461e087c02
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8560692 - Flags: review?(vporof)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8560692 [details] [diff] [review]
1111020-fix-key-events.patch

Review of attachment 8560692 [details] [diff] [review]:
-----------------------------------------------------------------

Wow

::: browser/devtools/performance/test/browser_perf-events-calltree.js
@@ +34,5 @@
> +  fireKey("VK_DOWN");
> +
> +  JsCallTreeView.off("focus", focusEvent);
> +
> +  ok(focus > 1, "several focus events are fired for the js calltree.");

Shouldn't we check for an exact number here?

@@ +52,5 @@
> +  fireKey("VK_UP");
> +  fireKey("VK_DOWN");
> +
> +  MemoryCallTreeView.off("focus", focusEvent);
> +  ok(focus > 1, "several focus events are fired for the memory calltree.");

Ditto.

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +447,5 @@
>  
>      switch (e.keyCode) {
>        case e.DOM_VK_UP:
>          this._focusPrevNode();
> +        e.preventDefault();

Huh? Why?
Attachment #8560692 - Flags: review?(vporof) → review+
Comment on attachment 8560692 [details] [diff] [review]
1111020-fix-key-events.patch

Review of attachment 8560692 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/test/browser_perf-events-calltree.js
@@ +34,5 @@
> +  fireKey("VK_DOWN");
> +
> +  JsCallTreeView.off("focus", focusEvent);
> +
> +  ok(focus > 1, "several focus events are fired for the js calltree.");

Will we always have sufficient enough call tree items? That's my concern as the stack generated isn't deterministic, I believe

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +447,5 @@
>  
>      switch (e.keyCode) {
>        case e.DOM_VK_UP:
>          this._focusPrevNode();
> +        e.preventDefault();

Not necessary for this patch, but to prevent sideeffects in other views from occuring possibly -- want me to remove?
(Reporter)

Comment 5

4 years ago
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> Comment on attachment 8560692 [details] [diff] [review]
> 1111020-fix-key-events.patch
> 
> Review of attachment 8560692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/performance/test/browser_perf-events-calltree.js
> @@ +34,5 @@
> > +  fireKey("VK_DOWN");
> > +
> > +  JsCallTreeView.off("focus", focusEvent);
> > +
> > +  ok(focus > 1, "several focus events are fired for the js calltree.");
> 
> Will we always have sufficient enough call tree items? That's my concern as
> the stack generated isn't deterministic, I believe
> 

Sounds like we should populate the tree manually.

> ::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
> @@ +447,5 @@
> >  
> >      switch (e.keyCode) {
> >        case e.DOM_VK_UP:
> >          this._focusPrevNode();
> > +        e.preventDefault();
> 
> Not necessary for this patch, but to prevent sideeffects in other views from
> occuring possibly -- want me to remove?

"possibly" is a weird reason. I'd steer clear from changes like these, but it's up to you.
Created attachment 8561584 [details] [diff] [review]
1111020-fix-key-events.patch

Manually populating the call tree view, and removed the preventDefaults
Attachment #8560692 - Attachment is obsolete: true
Attachment #8561584 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0dab1771c9b7
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0dab1771c9b7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.