Last Comment Bug 771481 - "No scripts." should be displayed in the dropdown when there are no scripts matching the search string
: "No scripts." should be displayed in the dropdown when there are no scripts m...
Status: RESOLVED FIXED
[testday-20120706]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 15 Branch
: All All
: P3 normal (vote)
: Firefox 17
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks: 774220
  Show dependency treegraph
 
Reported: 2012-07-06 05:21 PDT by Ioana (away)
Modified: 2012-07-19 07:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.35 KB, patch)
2012-07-17 23:35 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (7.20 KB, patch)
2012-07-18 00:06 PDT, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
v2.1 (8.35 KB, patch)
2012-07-18 06:16 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Ioana (away) 2012-07-06 05:21:15 PDT
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) 2012-07-12 09:17:02 PDT
Victor, wanna take this? Should probably read "No matching scripts" or something.
Comment 2 Victor Porof [:vporof][:vp] 2012-07-15 07:31:25 PDT
Okay.
Comment 3 Victor Porof [:vporof][:vp] 2012-07-17 23:35:21 PDT
Created attachment 643261 [details] [diff] [review]
v1
Comment 4 Victor Porof [:vporof][:vp] 2012-07-18 00:06:07 PDT
Created attachment 643269 [details] [diff] [review]
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 [:past] 2012-07-18 01:37:05 PDT
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 [:past] 2012-07-18 01:37:56 PDT
Comment on attachment 643269 [details] [diff] [review]
v2

past - bugzilla: 1 - 1
Comment 7 Victor Porof [:vporof][:vp] 2012-07-18 03:10:02 PDT
(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 [:past] 2012-07-18 03:38:20 PDT
(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] 2012-07-18 06:16:41 PDT
Created attachment 643345 [details] [diff] [review]
v2.1

Addressed comments.
Fixing the menupopup sizing issue by adding a "sizetopopup" = always attribute.
Comment 10 Panos Astithas [:past] 2012-07-18 07:08:18 PDT
https://hg.mozilla.org/integration/fx-team/rev/c55c28ca8d04
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-07-19 07:40:47 PDT
https://hg.mozilla.org/mozilla-central/rev/c55c28ca8d04

Note You need to log in before you can comment on or make changes to this bug.