771481 - "No scripts." should be displayed in the dropdown when there are no scripts matching the search string

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Ioana (away)]

Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20120705 Firefox/15.0a2 (20120705042010)

STR:
1. Load a web page with multiple scripts in the browser.
2. Open the Debugger.
3. Enter a string that doesn't match any of the scripts' names in the "Filter scripts" textbox.

"No scripts." should be displayed in the dropdown since there are no scripts matching the search string. Instead of this, the last selected script is displayed.

Comment 1

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

Victor, wanna take this? Should probably read "No matching scripts" or something.

Comment 2

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

Okay.

Comment 3

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

Attachment: v1

Comment 4

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

Attachment: v2

On this note, we should also display "No scripts" after navigating to a page with no scripts, instead of leaving the menulist completely naked like we do now.
Added that + more assertions in tests.

Comment 5

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

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

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

A few minor issues. r=me with those.
I'd have set the flag to r+ if bugzilla would let me.

::: browser/devtools/debugger/debugger-view.js
@@ +483,5 @@
>            item.hidden = true;
>          }
>        }
> +      if (!found) {
> +        scripts.setAttribute("label", L10N.getStr("noMatchingScriptsText"));

You need to set the tooltip to the same string as well, because otherwise it retains any previously held value. If the string was displayed in full of course, clearing the tooltip would suffice.

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +60,5 @@
>  emptyBreakpointsText=No breakpoints to display.
>  
> +# LOCALIZATION NOTE (noScriptsText): The text to display in the menulist when
> +# there are no scripts.
> +noScriptsText=No scripts.

The full stop doesn't look good in a menu list I think.

@@ +64,5 @@
> +noScriptsText=No scripts.
> +
> +# LOCALIZATION NOTE (noMatchingScriptsText): The text to display in the
> +# menulist when there are no matching scripts after filtering.
> +noMatchingScriptsText=No matching scripts.

Ditto.

::: browser/themes/gnomestripe/devtools/debugger.css
@@ +18,4 @@
>  
>  #scripts {
>    max-width: 350px;
> +  min-width: 150px;

Can we get this big enough to hold the 'no match' string without an ellipsis?

Comment 6

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

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

past - bugzilla: 1 - 1

Comment 7

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

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 643269 [details] [diff] [review]
> v2
> 
> Review of attachment 643269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few minor issues. r=me with those.
> I'd have set the flag to r+ if bugzilla would let me.
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +483,5 @@
> >            item.hidden = true;
> >          }
> >        }
> > +      if (!found) {
> > +        scripts.setAttribute("label", L10N.getStr("noMatchingScriptsText"));
> 
> You need to set the tooltip to the same string as well, because otherwise it
> retains any previously held value. If the string was displayed in full of
> course, clearing the tooltip would suffice.

You're right.
How about just clearing the tooltiptext though, since having the same tooltip as the contents is not really necessary?

Comment 8

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

(In reply to Victor Porof from comment #7)
> (In reply to Panos Astithas [:past] from comment #5)
> > Comment on attachment 643269 [details] [diff] [review]
> > v2
> > 
> > Review of attachment 643269 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > A few minor issues. r=me with those.
> > I'd have set the flag to r+ if bugzilla would let me.
> > 
> > ::: browser/devtools/debugger/debugger-view.js
> > @@ +483,5 @@
> > >            item.hidden = true;
> > >          }
> > >        }
> > > +      if (!found) {
> > > +        scripts.setAttribute("label", L10N.getStr("noMatchingScriptsText"));
> > 
> > You need to set the tooltip to the same string as well, because otherwise it
> > retains any previously held value. If the string was displayed in full of
> > course, clearing the tooltip would suffice.
> 
> You're right.
> How about just clearing the tooltiptext though, since having the same
> tooltip as the contents is not really necessary?

Sure. Like I said, if the string is legible, clearing the tooltip is sufficient. Right now all I see is "No ma...".

Comment 9

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

Attachment: v2.1

Addressed comments.
Fixing the menupopup sizing issue by adding a "sizetopopup" = always attribute.

Comment 10

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

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c55c28ca8d04