Closed Bug 843187 Opened 7 years ago Closed 7 years ago

Variables view: going down through the properties via keyboard is really broken

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: msucan, Assigned: jryans)

References

Details

Attachments

(1 file, 3 obsolete files)

Whenever I use End/Page Down/Down I see really bad behavior in the Variables View - it's unusable by keyboard.

Examples:

(1) when I hold Down for a long time focus jumps to the tab bar and that results in me switching to different tabs (!);

(2) End never goes to the last scope/variable/property, it even jumps back a few properties if you are close enough to the last item;

(3) Page Down goes down a few properties but it stops working properly once you are close to the end.
(In reply to Mihai Sucan [:msucan] from comment #0)

Wow, what happened? I remember this stuff working properly :(
Taking.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(In reply to Mihai Sucan [:msucan] from comment #0)
> Whenever I use End/Page Down/Down I see really bad behavior in the Variables
> View - it's unusable by keyboard.
> 
> Examples:
> 
> (1) when I hold Down for a long time focus jumps to the tab bar and that
> results in me switching to different tabs (!);
> 

I can't reproduce this.

> (2) End never goes to the last scope/variable/property, it even jumps back a
> few properties if you are close enough to the last item;
> 

It jumps back to the last enumerable property. Bug.

> (3) Page Down goes down a few properties but it stops working properly once
> you are close to the end.

Ditto.
Victor, I am willing to check this one out if you'd like.  Are you actively working on it?
That would be most excellent, be my guest! Thanks :)
My first guess would be to look into the logic behind focusLastVisibleNode and friends, see http://dxr.mozilla.org/mozilla-central/browser/devtools/shared/VariablesView.jsm#l894 There's probably something weird going on inside, I suspect the fact that we're using two different container boxes for enumerable and non-enumerable properties has something to do with it.
Assignee: vporof → jryans
Attached patch Patch v1 (obsolete) — Splinter Review
Indeed, as you suspected, this was related to the divide between enum and non-enum.  Because of that, the order of items in currHierarchy no longer matches the the visible order of variables and properties.

Added fix and tests.  All debugger tests pass locally.
Attachment #739316 - Flags: review?(vporof)
Comment on attachment 739316 [details] [diff] [review]
Patch v1

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

Good, I'm voting you the new VariablesView champion :) r+ with the comments below addressed.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +136,5 @@
>  
>      this._store.clear();
>      this._itemsByElement.clear();
> +    this._enumItems = [];
> +    this._nonenumItems = [];

You don't need to instantiate these here. AFAICT, those arrays (on a VariablesView instance) are never populated. Instead, the ones defined on a Scope (and thus, on a Variable and Property too) are.

@@ +339,5 @@
>    /**
> +   * Gets the last focusable child of this item.
> +   * @return Scope
> +   */
> +  get _lastFocusableChild() {

It'd be nice to move this getter closer to (above?) VV_focusLastVisibleNode to keep related things together.

It would also make me really happy if you'd create a similar logic for a _firstFocusableChild getter and reuse it in VV_focusFirstVisibleNode.

@@ +342,5 @@
> +   */
> +  get _lastFocusableChild() {
> +    let lastItem;
> +
> +    for (let [, item] in this) {

s/item/scope/ here and use "for (let [, scope] of this._store) {" to avoid the custom iterator overhead. Dealing with private stuff is ok, as long as it's only inside VariablesView.jsm.

@@ +592,2 @@
>      }
> +

Uber nit: extra line, meh.

@@ +1712,5 @@
>    /**
> +   * Gets the last focusable child of this item.
> +   * @return Variable | Property
> +   */
> +  get _lastFocusableChild() {

Nit: I would define this getter under "get _firstMatch()".

@@ +1715,5 @@
> +   */
> +  get _lastFocusableChild() {
> +    let lastItem;
> +
> +    if (this._variablesView._nonEnumVisible) {

This algorithm will only work properly if enumerable variables are always shown above non-enumerable ones. Please document this (it's pretty subtle for newcomers) at the beginning of the getter (or before this conditional).

I'm just thinking out loud here: it would sure be faster to loop over these the other way around, wouldn't it? How about using a plain old for (let i..) loop and start at the end?
Attachment #739316 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #6)

Hmm, I've tested this patch with webconsole and it doesn't seem to work as well there.
STR: evaluate |window|, inspect it and try out Page Up/ Page Down/ Home/ End. Debugger works perfect though. I suspect maybe the fact that webconsole is skipping a variable container when building the variables view is the culprit?
Ah yeah, great catch! 

I wasn't expecting the use case of a hidden variable container but with visible properties underneath it.  I'll come up with a revised algorithm. :)
Attached patch Patch v2 (obsolete) — Splinter Review
Okay, I've reworked the approach here.  I'm now flattening the tree view on demand into an array that's sorted in visual order.  I considered building a series of delegating iterators, but that felt like over-engineering the problem, at least at the moment when we don't yet have support for "yield*" in the JS engine.  These focusFirst / focusLast operations obviously don't happen frequently (only on HOME / END), so the expense of building a new array doesn't seem too bad to me.  I'm happy to file a bug to improve this once "yield*" support is done.

I've tested this version on both the web console and the debugger, and it seems to work as expected.  Also, your review comments from last time should all be addressed.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a7dd599ec0ea
Attachment #739316 - Attachment is obsolete: true
Attachment #740532 - Flags: review?(vporof)
Comment on attachment 740532 [details] [diff] [review]
Patch v2

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

Btw, yield is already supported. Check out the __iterator__s at the bottom of the file. But I agree, using them would be a tad bit overly-engineered.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +537,5 @@
> +   *        The array into which the items will be pushed.
> +   */
> +  _collectVisibleItems: function VV__collectVisibleItems(aArray) {
> +    for (let [, scope] of this._store) {
> +      scope._collectVisibleItems(aArray);

I'm relatively ok with this approach, but do you really need to create a new array just to loop over it again? For the HOME scenario, you'll loop over all nodes, just to potentially find a focusable node as the first item! That's.. brutal :)

One way of optimizing this would be to have two methods: getFirstFocusableNode and getLastFocusableNode in the Scope and Variable prototypes and return immediately once the wanted node is found.

Another idea would be to pass on a breakCallback to the collectVisibleItems method, invoked on each node, that breaks the loop internally once it returns true. This callback would check if the item is focusable. I think this is the most elegant solution, but the overhead of invoking a function on each node would be noticeable.

I'm ok with followups if you so desire.

Also, if you think my concerns are invalid and this works OK for you when testing on a VariablesView instance with 10,000 nodes, then it's all good :) Just please try to do some stress testing and see how this behaves.
Attachment #740532 - Flags: review?(vporof) → review+
I've tested with 30,000 elements and there are about 2 seconds delay between me pressing HOME and the variables view scrolling. How's the timing on your side? I do agree this is an edge case scenario though..
(In reply to Victor Porof [:vp] from comment #10)
> 
> One way of optimizing this would be to have two methods:
> getFirstFocusableNode and getLastFocusableNode in the Scope and Variable
> prototypes and return immediately once the wanted node is found.
> 

If I think about it more, this would work amazing with getFirstFocusableNode but *really* suck for getLastFocusableNode because checking if (item.focusable) on each node would be a nightmare!

Also, like I said, the second idea (with the breakCallback) would also be bad because overhead.

Man, just leave it as it is, it's all good :)
(In reply to Victor Porof [:vp] from comment #10)
> Btw, yield is already supported. Check out the __iterator__s at the bottom
> of the file. But I agree, using them would be a tad bit overly-engineered.

Simple yield is supported, but I was thinking of "yield*", which delegates to another generator: http://wiki.ecmascript.org/doku.php?id=harmony:generators#delegating_yield

All kinds of strange generators could be made once we have that supported... ;)
Attached patch Patch v3 (obsolete) — Splinter Review
This approach is based on your callback idea.  Now we don't need to construct any arrays to find the first or last items anymore.  With separate modes for forward and reverse, both directions are quick to find the right focusable item (~0 ms with 30,000 items).

The actual focusing / scrolling step can be slow (~700 ms with 30,000 items), but that was still the case before.

Looking at this change in the profiler, I don't see any impact caused by the extra array operations when adding new items to the view, so it seems fine from that perspective.

Tested locally in both the web console and debugger.
Attachment #740532 - Attachment is obsolete: true
Attachment #740736 - Flags: review?(vporof)
(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> 
> Simple yield is supported, but I was thinking of "yield*", which delegates
> to another generator:
> http://wiki.ecmascript.org/doku.php?id=harmony:generators#delegating_yield
> 
> All kinds of strange generators could be made once we have that supported...
> ;)

Ah, delicious!
Comment on attachment 740736 [details] [diff] [review]
Patch v3

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

Lovely. Just a few nits and another optimization idea for r+. Great job!

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +51,5 @@
>   *        e.g. { lazyEmpty: true, searchEnabled: true ... }
>   */
>  this.VariablesView = function VariablesView(aParentNode, aFlags = {}) {
>    this._store = new Map();
> +  this._items = [];

s/items/itemsAsArray/ is more descriptive maybe?

@@ +136,5 @@
>        list.removeChild(firstChild);
>      }
>  
>      this._store.clear();
> +    this._items = [];

this._items.length = 0 to save some memory.

@@ +163,5 @@
>      let prevList = this._list;
>      let currList = this._list = this.document.createElement("scrollbox");
>  
>      this._store.clear();
> +    this._items = [];

Ditto.

@@ +537,5 @@
> +   * matches the predicate. Searches in visual order (the order seen by the
> +   * user). Descends into each scope to check the scope and its children.
> +   *
> +   * @param function aPredicate
> +   *        A function that returns true when a match is found.

Document @return please.

@@ +556,5 @@
> +   * order seen by the user). Descends into each scope to check the scope and
> +   * its children.
> +   *
> +   * @param function aPredicate
> +   *        A function that returns true when a match is found.

Ditto.

@@ +575,5 @@
>     */
>    focusFirstVisibleNode: function VV_focusFirstVisibleNode() {
> +    let focusableItem = this._findInVisibleItems(function(item) {
> +      return item.focusable;
> +    });

let focusableItem = this._findInVisibleItems(item => item.focusable);
LIKE A BOSS.

@@ +590,5 @@
>     */
>    focusLastVisibleNode: function VV_focusLastVisibleNode() {
> +    let focusableItem = this._findInVisibleItemsReverse(function(item) {
> +      return item.focusable;
> +    });

Ditto.

@@ +1805,5 @@
> +   * Tests itself, then descends into first the enumerable children and then
> +   * the non-enumerable children (since they are presented in separate groups).
> +   *
> +   * @param function aPredicate
> +   *        A function that returns true when a match is found.

Document @return please.

@@ +1810,5 @@
> +   */
> +  _findInVisibleItems: function S__findInVisibleItems(aPredicate) {
> +    if (aPredicate(this)) {
> +      return this;
> +    }

Adding the following check here will speed up things when a node with tons of children is collapsed (and also stays true to the "visible items" promise):

if (this._isExpanded) {
  .. all the loops!
}

@@ +1841,5 @@
> +   * the enumerable children (since they are presented in separate groups), and
> +   * finally tests itself.
> +   *
> +   * @param function aPredicate
> +   *        A function that returns true when a match is found.

Ditto for documenting @return.

@@ +1843,5 @@
> +   *
> +   * @param function aPredicate
> +   *        A function that returns true when a match is found.
> +   */
> +  _findInVisibleItemsReverse: function S__findInVisibleItemsReverse(aPredicate) {

Ditto for the expansion check.

if (this._isExpanded) {
  .. all the loops!
}
Attachment #740736 - Flags: review?(vporof) → review+
Attached patch Patch v4Splinter Review
All review comments addressed.  Carrying over r+.
Attachment #740736 - Attachment is obsolete: true
Attachment #740768 - Flags: review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/45829ebfd8f4
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/45829ebfd8f4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.