I can make it look similar to what Inspector's autocomplete panel will look like in a few days. or.. something else. Lets decide.

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.

Screenshot of Light Theme Popup. Patches incoming.

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 :)

Removed one useless change that I forgot to remove in the previous patch.

Patch for dark theme of the popup

Patch for light theme of the popup More reviews :)

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 :( )

Since we will be forcing ltr direction, had to update here too.

addressed comments that I got in the similar style in bug 831693

addressed comments that I got in the similar style in bug 831693

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?

(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.

Carry forward r+. Fixed the test descriptions.

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.

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.

(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.

(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).

(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.

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.

Shared every style into a new file common.inc.css This can also used for sharing more stuff. (not test, Clobbering)

Uploaded a wrong file previously.

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.

This works. Tested on Windows 7. All review comments addressed.

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.

(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.

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)

Review comments addressed.

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 :) )

combined the two patches. rebased too.

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.