Closed Bug 835899 Opened 9 years ago Closed 9 years ago

Web Console autocomplete popup could need some UI love

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(firefox22 verified)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox22 --- verified

People

(Reporter: Optimizer, Assigned: Optimizer)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(4 files, 14 obsolete files)

103.02 KB, image/png
Details
87.41 KB, image/png
Details
270 bytes, text/html
shorlander
: ui-review+
Details
21.31 KB, patch
Optimizer
: review+
Details | Diff | Splinter Review
I can make it look similar to what Inspector's autocomplete panel will look like in a few days.

or.. something else. 

Lets decide.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Depends on: 831693
OS: Windows 7 → All
Hardware: x86_64 → All
Attached image Dark Theme.
Screenshot of dark theme on the Popup.

Light theme coming up.

PS: These styles are applied on the class and are declared in common.css, so any popup that implemented AutocompletePopup get these for free.
Attached image Light Theme.
Screenshot of Light Theme Popup.

Patches incoming.
Attached patch Web Console changes v1.0 (obsolete) — Splinter Review
Changes to web console popup (no UI changes)

This patch does:

- Auto selects first item of the popup fixing bug 833333 too (depends)
- Makes the popup results alphabetically sorted starting from the input, as in the first lexical result will be down at bottom. (basically reversing the current items)
- Test fixes . Try at https://tbpl.mozilla.org/?tree=Try&rev=6070e9aa41e5


Theme changes are in 2 separate patches. One for Dark and one for Light, one of them is required and I will leave the decision and review for that on Paul :)
Attachment #712149 - Flags: review?(mihai.sucan)
Attached patch Web Console changes v1.1 (obsolete) — Splinter Review
Removed one useless change that I forgot to remove in the previous patch.
Attachment #712149 - Attachment is obsolete: true
Attachment #712149 - Flags: review?(mihai.sucan)
Attachment #712150 - Flags: review?(mihai.sucan)
Attached patch Dark Popup Theme v1.0 (obsolete) — Splinter Review
Patch for dark theme of the popup
Attachment #712152 - Flags: review?(paul)
Attached patch Light PopupTheme v1.0 (obsolete) — Splinter Review
Patch for light theme of the popup

More reviews :)
Attachment #712153 - Flags: review?(paul)
All the web console related tests are green on try, the 2 test failures that each build is facing have been fixed in the tests for bug 831693 (which I forgot to include here :( )
Attached patch Web Console changes v1.2 (obsolete) — Splinter Review
Since we will be forcing ltr direction, had to update here too.
Attachment #712150 - Attachment is obsolete: true
Attachment #712150 - Flags: review?(mihai.sucan)
Attachment #714821 - Flags: review?(mihai.sucan)
Attached patch Dark Popup Theme v1.1 (obsolete) — Splinter Review
addressed comments that I got in the similar style in bug 831693
Attachment #712152 - Attachment is obsolete: true
Attachment #712152 - Flags: review?(paul)
Attachment #714825 - Flags: review?(paul)
Attached patch Light PopupTheme v1.1 (obsolete) — Splinter Review
addressed comments that I got in the similar style in bug 831693
Attachment #712153 - Attachment is obsolete: true
Attachment #712153 - Flags: review?(paul)
Attachment #714828 - Flags: review?(paul)
Optimizer: we discussed having a max-height for the web console autocomplete popup.

Here's how it looks for me:
http://img.i7m.de/show/qmb4t.png

Please add a max-height for the popup.
(In reply to Mihai Sucan [:msucan] from comment #11)
> Optimizer: we discussed having a max-height for the web console autocomplete
> popup.
> 
> Here's how it looks for me:
> http://img.i7m.de/show/qmb4t.png
> 
> Please add a max-height for the popup.

You have to apply any of the other two patches. (light or dark)
Comment on attachment 714821 [details] [diff] [review]
Web Console changes v1.2

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

r+ with the concerns below addressed, and a green try push.

::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js
@@ +63,5 @@
>          "valueOf",
>          "watch",
>        ][index] === prop}), "getItems returns the items we expect");
>  
> +    is(popup.selectedIndex, 17, "no index is selected");

wrong test description here.

::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_popup.js
@@ +40,5 @@
>        return aItem === items[aIndex];
>      }), true, "getItems returns back the same items");
>  
> +    is(popup.selectedIndex, 2, "no index is selected");
> +    is(popup.selectedItem, items[2], "First item from bottom is selected");

what is the intent of the test here? are these descriptions correct?
Attachment #714821 - Flags: review?(mihai.sucan) → review+
(In reply to Girish Sharma [:Optimizer] from comment #12)
> (In reply to Mihai Sucan [:msucan] from comment #11)
> > Optimizer: we discussed having a max-height for the web console autocomplete
> > popup.
> > 
> > Here's how it looks for me:
> > http://img.i7m.de/show/qmb4t.png
> > 
> > Please add a max-height for the popup.
> 
> You have to apply any of the other two patches. (light or dark)

Thanks! I forgot to do that.
(In reply to Mihai Sucan [:msucan] from comment #13)
> Comment on attachment 714821 [details] [diff] [review]
> Web Console changes v1.2
> 
> Review of attachment 714821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the concerns below addressed, and a green try push.
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_585991_autocomplete_keys.js
> @@ +63,5 @@
> >          "valueOf",
> >          "watch",
> >        ][index] === prop}), "getItems returns the items we expect");
> >  
> > +    is(popup.selectedIndex, 17, "no index is selected");
> 
> wrong test description here.

my bad.

> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_585991_autocomplete_popup.js
> @@ +40,5 @@
> >        return aItem === items[aIndex];
> >      }), true, "getItems returns back the same items");
> >  
> > +    is(popup.selectedIndex, 2, "no index is selected");
> > +    is(popup.selectedItem, items[2], "First item from bottom is selected");
> 
> what is the intent of the test here? are these descriptions correct?

The intent is that earlier no suggestion was selected automatically. Now the first one will be. So this test is just checking that. Note: first one means the last index here.
And yes, the descriptions are incorrect.
Attached patch Web Console changes v1.3 (obsolete) — Splinter Review
Carry forward r+.
Fixed the test descriptions.
Attachment #714821 - Attachment is obsolete: true
Attachment #714893 - Flags: review+
Clubbed both dark and light themes, added an option in the autocomplete popup to choose a theme.

Also moved the optioin variable's default assignment outside of function definition line as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=831693#c48

Did not want to change the patch there as it is at final stages of review.
Attachment #714825 - Attachment is obsolete: true
Attachment #714828 - Attachment is obsolete: true
Attachment #714825 - Flags: review?(paul)
Attachment #714828 - Flags: review?(paul)
Attachment #714919 - Flags: review?(paul)
The order of patches to apply is:

patches from bug 831693
Web Console changes v1.3 
Combined theme - both dark and light
Comment on attachment 714919 [details] [diff] [review]
Combined theme - both dark and light

the popup should try to be consistent with the look in the popup for the debugger's search field (for the light theme). We've got multiple different popup styles emerging and we should try to keep them similar.
Attachment #714919 - Flags: review-
(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> Comment on attachment 714919 [details] [diff] [review]
> Combined theme - both dark and light
> 
> the popup should try to be consistent with the look in the popup for the
> debugger's search field (for the light theme). We've got multiple different
> popup styles emerging and we should try to keep them similar.

I beg to differ. Debugger's popup has completely different functionality.

1) It does not suggest the part that will be auto completed.
2) It has two lines per item.
3) It searches for the query in any part of the script url.

The most closest Web Console's popup is to the inspector's search suggestion popup and in future, the scratchpad auto complete popup. The all have and will have the same UI (except for the fact that Web console right now needs a light UI while inspector search box needs a dark UI).

This has all been discussed with Paul over IRC.

If we want to change, we should change the look of the debugger's search popup to somehow match the dark theme (as the search box is on a dark toolbar) as done for inspector's search box.
Comment on attachment 714919 [details] [diff] [review]
Combined theme - both dark and light

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

Went through the patch, found only one small issue:

::: browser/devtools/shared/AutocompletePopup.jsm
@@ +40,5 @@
>  
> +  this.fixedWidth = aOptions.fixedWidth || false;
> +  this.autoSelect = aOptions.autoSelect || false;
> +  this.position = aOptions.position || "after_start";
> +  this.direction = aOptions.direction || "ltr";

this.direction || ""; because you want to allow the default browser UI direction for the popup, unless the AutocompletePopup user wants to explicitly override the direction.
(In reply to Mihai Sucan [:msucan] from comment #21)
> Comment on attachment 714919 [details] [diff] [review]
> Combined theme - both dark and light
> 
> Review of attachment 714919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Went through the patch, found only one small issue:
> 
> ::: browser/devtools/shared/AutocompletePopup.jsm
> @@ +40,5 @@
> >  
> > +  this.fixedWidth = aOptions.fixedWidth || false;
> > +  this.autoSelect = aOptions.autoSelect || false;
> > +  this.position = aOptions.position || "after_start";
> > +  this.direction = aOptions.direction || "ltr";
> 
> this.direction || ""; because you want to allow the default browser UI
> direction for the popup, unless the AutocompletePopup user wants to
> explicitly override the direction.

Agreed.
Attachment #714919 - Flags: ui-review?(shorlander)
(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> We've got multiple different
> popup styles emerging and we should try to keep them similar.

That's the point of this patch (the web console, the inspector and the gcli will have more or less the same look, or at least, it will be a step in the right direction).

> the popup should try to be consistent with the look in the popup for the
> debugger's search field (for the light theme).

Hmmm, maybe it could be the other way around?
Screenshots for Linux and osx: http://imgur.com/a/Xngi0
On Linux, the popup max-height should not be 40em, but 32rem:

Screenshot with 32rem: http://i.imgur.com/CHhxBJg.png

(you should use REM instead of EM).
Will update the patch after the next m-c -> fx-team merge, as the themes directory path is changed.
For future compatibility with the dark/light theme mechanism, could you use:

instead of: devtools-dark/light-autocomplete-popup
use:        dark/light-theme
Comment on attachment 714919 [details] [diff] [review]
Combined theme - both dark and light



> /browser/devtools/inspector/inspector.css
>-#inspector-searchbox-panel {
>-  -moz-appearance: none !important;
>-  border: 1px solid hsl(210,24%,10%);
>-  box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
>-  background-color: transparent;
>-  background-image: linear-gradient(to bottom, hsla(209,18%,18%,0.9), hsl(210,24%,16%));
>-  border-radius: 3px;
>-}
>-
> #searchbox-panel-listbox {
>-  -moz-appearance: none !important;
>-  background-color: rgba(0,0,0,0);
>-  border-width: 0px !important;
>   width: 250px;
>   max-width: 250px;
>   overflow-x: hidden;
> }

Isn't overflow-x:hidden something we want to share too?
Is it inspector specific?

> a/browser/devtools/shared/AutocompletePopup.jsm
>+    this._panel.setAttribute("class", "devtools-" + theme + "-autocomplete-popup");

As discussed on IRC, I'd prefer if you could do:

> .setAttribute("class", "devtools-autocomplete-popup theme-" + theme);

>-  this._list.setAttribute("class", "devtools-autocomplete-listbox");
>+  this._list.setAttribute("class", "devtools-" + theme + "-autocomplete-listbox");

Ditto.

This applies to all the themes (gnome/pin/win):

> b/browser/themes/gnomestripe/devtools/common.css
>+/* Autocomplete Popup */
>+/* Dark and light theme */
>+
>+.devtools-dark-autocomplete-popup,
>+.devtools-light-autocomplete-popup {

.devtools-autocomplete-popup {
   [...]
}

>+.devtools-dark-autocomplete-listbox,
>+.devtools-light-autocomplete-listbox {

.devtools-autocomplete-listbox {}

>+  -moz-appearance: none !important;
>+  background-color: rgba(0,0,0,0);

transparent?

>+.devtools-dark-autocomplete-listbox > richlistitem,
>+.devtools-light-autocomplete-listbox > richlistitem,
>+.devtools-dark-autocomplete-listbox > richlistitem[selected],
>+.devtools-light-autocomplete-listbox > richlistitem[selected] {

.devtools-autocomplete-listbox > richlistitem,
.devtools-autocomplete-listbox > richlistitem[selected] {}


>+  width: 100%;
>+  background-color: rgba(0,0,0,0);

transparent.

>+.devtools-dark-autocomplete-listbox > richlistitem[selected],
>+.devtools-dark-autocomplete-listbox > richlistitem:hover {

To update.

>+  background-color: rgba(0,0,0,0.2);
>+}
>+
>+.devtools-dark-autocomplete-listbox > richlistitem[selected] > .autocomplete-value {
>+  color: hsl(208,100%,60%);
>+}

To update (you get it. All the selectors need to be updated).
Attachment #714919 - Flags: ui-review?(shorlander)
Attachment #714919 - Flags: review?(paul)
Attachment #714919 - Flags: review-
(In reply to Paul Rouget [:paul] from comment #28)
> Comment on attachment 714919 [details] [diff] [review]
> Combined theme - both dark and light
> 
> 
> 
> > /browser/devtools/inspector/inspector.css
> >-#inspector-searchbox-panel {
> >-  -moz-appearance: none !important;
> >-  border: 1px solid hsl(210,24%,10%);
> >-  box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
> >-  background-color: transparent;
> >-  background-image: linear-gradient(to bottom, hsla(209,18%,18%,0.9), hsl(210,24%,16%));
> >-  border-radius: 3px;
> >-}
> >-
> > #searchbox-panel-listbox {
> >-  -moz-appearance: none !important;
> >-  background-color: rgba(0,0,0,0);
> >-  border-width: 0px !important;
> >   width: 250px;
> >   max-width: 250px;
> >   overflow-x: hidden;
> > }
> 
> Isn't overflow-x:hidden something we want to share too?
> Is it inspector specific?

Yes, since web console has a dynamic width as it is showing JS properties and stuff and the input box for web console is of 100% width. In inspector, synamic width does not make much sense as the search box is of 150 px only.

Agreed for rest all.
Attached file screenshots
Stephen, can you please take a look at these screenshots. We have updated the style of the web console popup and you can also see the new inspector popup.
Attachment #717867 - Flags: ui-review?(shorlander)
Attached patch shared styles. (obsolete) — Splinter Review
Shared every style into a new file common.inc.css 
This can also used for sharing more stuff.

(not test, Clobbering)
Attachment #714919 - Attachment is obsolete: true
Attachment #717953 - Flags: review?(paul)
Attached patch shared styles v2 (obsolete) — Splinter Review
Uploaded a wrong file previously.
Attachment #717953 - Attachment is obsolete: true
Attachment #717953 - Flags: review?(paul)
Attachment #717956 - Flags: review?(paul)
Comment on attachment 717956 [details] [diff] [review]
shared styles v2

removing the r? as the patch does not build correctly. Will upload a correct version soon.
Attachment #717956 - Flags: review?(paul)
Attached patch shared styles v3 (obsolete) — Splinter Review
This works. Tested on Windows 7. All review comments addressed.
Attachment #717956 - Attachment is obsolete: true
Attachment #718250 - Flags: review?(paul)
Comment on attachment 717867 [details]
screenshots

I think it looks good.

My only thought here is the there is a discrepancy between selected item treatment between light and dark popups.

I would pick a treatment that works both places. Either use the same hue and contrast approach on both or use blue on both. E.g. on the dark one make all of the non-match slightly more subdued and darken the selected background-color.

A third options would be to use a fairly standard blue selected background with inverted(white) text.

I also don't see a reason that the matched text should be a different color than blue.
Attachment #717867 - Flags: ui-review?(shorlander) → ui-review+
Attached patch shared styles v4 (obsolete) — Splinter Review
(In reply to Stephen Horlander from comment #35)
> Comment on attachment 717867 [details]
> screenshots
> 
> I think it looks good.

Thanks :)

> My only thought here is the there is a discrepancy between selected item
> treatment between light and dark popups.

True, I also don't like what I have to do for the selected item on light theme. But we can assume that Web console will also go dark when the switching mechanism is in place ;)

> I would pick a treatment that works both places. Either use the same hue and
> contrast approach on both or use blue on both. E.g. on the dark one make all

I tried the same blue shade for the light theme popup and it was looking a bit out of place. Screenshot : http://i.imgur.com/RNnsZ0U.png?1

> of the non-match slightly more subdued and darken the selected
> background-color.

The background darkening was already present, but the contrast was not that much making it look like non selected entries. I have made the contrast better in this patch.

> A third options would be to use a fairly standard blue selected background
> with inverted(white) text.

That might not look that great on a dark theme popup : http://i.imgur.com/Dbj0XsH.png?1 but I can be wrong.

> I also don't see a reason that the matched text should be a different color
> than blue.

When the user is typing in the input box, the first suggestion is automatically selected, in that case, the suggested text is kept in a different color to suggest that this will be autocompleted on an actin like TAB or up key.
But yes, when the user is navigating in the list, they should be both of the same color. Fixed in this patch. (Although, since web console does not give focus to the panel, it will still have [grey][black] color pattern.
Attachment #718250 - Attachment is obsolete: true
Attachment #718250 - Flags: review?(paul)
Attachment #719092 - Flags: review?(paul)
Comment on attachment 719092 [details] [diff] [review]
shared styles v4

>-  this._list.setAttribute("class", "devtools-autocomplete-listbox");
>-
>+  this._list.setAttribute("class",
>+                          "devtools-autocomplete-listbox " + theme + "-theme");

this._list.className = "devtools-autocomplete-listbox " + theme + "-theme";

>+%include ../../shared/devtools/common.inc.css

Yeah!

:: browser/themes/shared/devtools/common.inc.css
>+.devtools-autocomplete-popup.dark-theme,
>+.devtools-autocomplete-popup.light-theme {
>+  […]
>+}

Because for now we only want a dark and a light theme, no need
to duplicate the selector. Just use:
.devtools-autocomplete-popup {}

(true for the rest of the file)
Attachment #719092 - Flags: review?(paul) → review-
Attached patch shared styles v5 (obsolete) — Splinter Review
Review comments addressed.
Attachment #719092 - Attachment is obsolete: true
Attachment #723075 - Flags: review?(paul)
gentle review ping :)

It would be awesome if this can land in this week, maintaining the pace of devtools workweek, this will also be an awesome addition to the tools (maybe another blog post by you :) )
Attachment #723075 - Flags: review?(paul) → review+
Whiteboard: [land-in-fx-team]
combined the two patches. rebased too.
Attachment #714893 - Attachment is obsolete: true
Attachment #723075 - Attachment is obsolete: true
Attachment #725017 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/ab77f1f91522
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ab77f1f91522
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Verified as fixed on Windows 7 64bit, Mac OSX 10.7 and Ubuntu 13.04 32bit, using Firefox 22b2 (20130521223249). The fix works as specified in comment 3.
Duplicate of this bug: 883225
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.