762452 - file filtering behaves strangely

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Kevin Dangoor]

STR:

1. open http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.html
2. open debugger
3. In filter box, type "todo"
4. *** Note that selected file is still backbone-localstorage.js, which doesn't make sense
5. add an "s" to the filter box
6. it jumps to todos.js
7. add "@rema" to filter box
8. it jumps to remaining: function()
9. add "i" to filter box
10. *** it goes back to the beginning of the file
11. add "n" to the filter box
12. it just back to remaining: function
13. add "i" to the filter box and it jumps back to the top

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Kevin Dangoor from comment #0)
> STR:
> 
> 1. open
> http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.
> html
> 2. open debugger
> 3. In filter box, type "todo"
> 4. *** Note that selected file is still backbone-localstorage.js, which
> doesn't make sense

Yes, we match on the URL, not the listbox label. That's an easy fix.

> 5. add an "s" to the filter box
> 6. it jumps to todos.js
> 7. add "@rema" to filter box
> 8. it jumps to remaining: function()
> 9. add "i" to filter box
> 10. *** it goes back to the beginning of the file
> 11. add "n" to the filter box
> 12. it just back to remaining: function
> 13. add "i" to the filter box and it jumps back to the top

These jumps to the top are weird though.

Comment 2

[Kevin Dangoor]

(In reply to Panos Astithas [:past] from comment #1)
> Yes, we match on the URL, not the listbox label. That's an easy fix.

Aha. That makes sense. Working from the listbox label will certainly be less surprising.

> > 5. add an "s" to the filter box
> > 6. it jumps to todos.js
> > 7. add "@rema" to filter box
> > 8. it jumps to remaining: function()
> > 9. add "i" to filter box
> > 10. *** it goes back to the beginning of the file
> > 11. add "n" to the filter box
> > 12. it just back to remaining: function
> > 13. add "i" to the filter box and it jumps back to the top
> 
> These jumps to the top are weird though.

Right, the file selection was a little surprising, but the bouncing back and forth was clearly a bug.

Comment 3

[Rob Campbell [:rc] (:robcee)]

I think we'll want this for aurora.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v1

Alright, so this works properly now, but it's a bit baffling to me.
Mihai, can you please take a look at the diff:

+      let currentOffset = editor.getCaretOffset();
       let offset = editor.find(token, { ignoreCase: true });
-      if (offset > -1) {
+
+      if (offset > -1 && offset !== currentOffset) {
         editor.setCaretPosition(0);
         editor.setCaretOffset(offset);
       }

Basically, as expected, the previous behavior would give me the same caret offset as the previous offset when I searched for something and found it (hence, the caret should've remained at the same position), but subsequent calls to setCaretPosition with the same param would make it jump to frenetically. Was this the expected behavior of the function?

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Kevin Dangoor from comment #0)

Just a note, in bug 762454 "@" was replaced with "#".

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 633796 [details] [diff] [review]
v1

This fix looks good to me. The setCaretPosition(0) call should be removed - just do setCaretOffset(offset). Or even better: setSelection(offset, offset + token.length).

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v2

Thanks msucan! Made the change.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 633819 [details] [diff] [review]
v2

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

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-01.js
@@ +178,4 @@
>    for (let i = 0; i < text.length; i++) {
>      EventUtils.sendChar(text[i]);
>    }
> +  dump("Editor caret position: " + gEditor.getCaretPosition().toSource() + "\n");

Nit: turn the dump() into info() if it's still useful.

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-02.js
@@ +144,4 @@
>    for (let i = 0; i < text.length; i++) {
>      EventUtils.sendChar(text[i]);
>    }
> +  dump("Editor caret position: " + gEditor.getCaretPosition().toSource() + "\n");

Ditto.

Comment 9

[Victor Porof [:vporof][:vp]]

Thanks!

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

No need to track this, just go ahead and nominate the patch for uplift when you're ready.

Comment 11

[Victor Porof [:vporof][:vp]]

Attachment: v3

Addressed comments.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/543030f7b43b

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/543030f7b43b

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 634425 [details] [diff] [review]
v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: the script filtering/selection box will misbehave in certain cases
Testing completed (on m-c, etc.): On m-c and fx-team
Risk to taking this patch (and alternatives if risky): it's a small patch (leaving tests aside) and concerns a tool that only developers will ever use
String or UUID changes made by this patch: none

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a67494fea93

Comment 16

[Ioana (away)]

I tried to verify this fix on Firefox 15 beta 3 (20120731150526) and it seems it's only fixed on Linux x64. On all the other OSs (Win 7 x86/x64, Linux x32, Mac 10.7 x86/x64), the filtering works fine until you add the "@", at which point it stops working.

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to Ioana Budnar [QA] from comment #16)
> I tried to verify this fix on Firefox 15 beta 3 (20120731150526) and it
> seems it's only fixed on Linux x64. On all the other OSs (Win 7 x86/x64,
> Linux x32, Mac 10.7 x86/x64), the filtering works fine until you add the
> "@", at which point it stops working.

We're not using @ anymore. Bug 762454.

Comment 18

[Victor Porof [:vporof][:vp]]

Also, the testpage (http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.html) has changed since comment #1 so the STR don't apply exactly the same way anymore.

Comment 19

[Ioana (away)]

(In reply to Victor Porof [:vp] from comment #18)
> Also, the testpage
> (http://addyosmani.github.com/todomvc/architecture-examples/backbone/index.
> html) has changed since comment #1 so the STR don't apply exactly the same
> way anymore.

Victor, I took that into account when verifying this fix. When entering "todos" in the filtering textbox, two scripts fit the filter and you have to manually choose "todos.js". After doing that, you can just follow the rest of the STR.

Either way, filtering with "@" works on Linux x64 (I used Ubuntu 12.04) and it doesn't work on the other OSs. This is an inconsistency that needs fixing. If you don't want to fix it here, let me know and I could reopen bug 762454 and mark this one as verified.

Comment 20

[Victor Porof [:vporof][:vp]]

(In reply to Ioana Budnar [QA] from comment #19)
> (In reply to Victor Porof [:vp] from comment #18)
> 
> Either way, filtering with "@" works on Linux x64 (I used Ubuntu 12.04) and
> it doesn't work on the other OSs. This is an inconsistency that needs
> fixing. If you don't want to fix it here, let me know and I could reopen bug
> 762454 and mark this one as verified.

That's... unlikely. If "@" would work then the tests on tbpl should be permaorange on Linux. What FF version are you testing with?

Comment 21

[Ioana (away)]

"@" worked for me no Ubuntu 12.04 64-bit:
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
BuildID: 20120731150526

I can't reproduce it too often though.