The default bug view has changed. See this FAQ.

file filtering behaves strangely

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Kevin Dangoor, Assigned: vporof)

Tracking

14 Branch
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox15 fixed)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(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.
(Reporter)

Comment 2

5 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
tracking-firefox15: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
I think we'll want this for aurora.
(Assignee)

Comment 4

5 years ago
Created attachment 633796 [details] [diff] [review]
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?
Attachment #633796 - Flags: review?(past)
(Assignee)

Updated

5 years ago
Attachment #633796 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 5

5 years ago
(In reply to Kevin Dangoor from comment #0)

Just a note, in bug 762454 "@" was replaced with "#".
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).
Attachment #633796 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 633819 [details] [diff] [review]
v2

Thanks msucan! Made the change.
Attachment #633796 - Attachment is obsolete: true
Attachment #633796 - Flags: review?(past)
Attachment #633819 - Flags: review?(past)
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.
Attachment #633819 - Flags: review?(past) → review+
(Assignee)

Comment 9

5 years ago
Thanks!
No need to track this, just go ahead and nominate the patch for uplift when you're ready.
tracking-firefox15: ? → -
(Assignee)

Comment 11

5 years ago
Created attachment 634425 [details] [diff] [review]
v3

Addressed comments.
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/543030f7b43b
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/543030f7b43b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
Attachment #633819 - Attachment is obsolete: true
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
Attachment #634425 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #634425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a67494fea93
status-firefox15: --- → fixed
tracking-firefox15: - → ---

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
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

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
(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

5 years ago
"@" 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.
You need to log in before you can comment on or make changes to this bug.