Implement the new design by shorlander from bug 959564

quick question if these two bugs are for the same thing... this and bug 1139902

looks like bug 1139902 is for an old version of the mock-up and mostly pointed towards adding search suggestions and functionality rather than implementing the visual design (indeed that mock-up is also missing measures) So I don't think it's the same. On the other side that bug is basically done, it's a breakdown bug and we're already working on implementation... I'll mark it as fixed.

I will start playing with this and see how far I can go.

Dupe of bug 1040923 ?

(In reply to Guillaume C. [:ge3k0s] from comment #4) > Dupe of bug 1040923 ? no, it's a different mock-up.

(In reply to Marco Bonardo [::mak] from comment #5) > (In reply to Guillaume C. [:ge3k0s] from comment #4) > > Dupe of bug 1040923 ? > > no, it's a different mock-up. I know but, since the mockup here is more recent, bug 1040923 can be marked as a duplicate of this one.

(In reply to Guillaume C. [:ge3k0s] from comment #6) > I know but, since the mockup here is more recent, bug 1040923 can be marked > as a duplicate of this one. Maybe, I don't know if UX wants to iterate further on that one though, so I am not making that call.

This new UI breaks existing custom search titles as it hides the custom search bookmark's title and only displays the URL. For example I have a Google Images custom search bookmark with the title 'Google Images' and another custom search 'Reverse Image Lookup', apart from dozens of "site:" Google.com searches. In the new UI these *all* display in the new addressbar drop-down as 'encrypted.google'. No title at all to differentiate them. Screenshots: http://i.imgur.com/1Y5c4JS.png http://i.imgur.com/nskeg5z.png

Thak you for your report, unfortunately Google differentiates searches through parameters rather than hosts (that would make many things easier), and keywords are no more bound to bookmarks from some versions (yes, the UI lies currently, faking keywords in the bookmarks dialog) so we can't use the bookmark title, that's why we fallback to the host. The good thing is that the keyword is well visible in the location bar, so you know what you are using, the host is just a reminder for which page you are searching on. In future we may decide to reintroduce keyword labels, but that requires a new keywords management UI (that is in the plans but no ETA yet). That said, this is already happening in 43 and not related to this specific redesign that will land in a later version.

(In reply to Marco Bonardo [::mak] from comment #9) > The good thing is that the keyword is well visible in the location bar, > so you know what you are using, the host is just a reminder for which page > you are searching on. > In future we may decide to reintroduce keyword labels, but that requires a > new keywords management UI (that is in the plans but no ETA yet). Thanks for the info, Marco. Wasn't aware they weren't a part of the same bookmark. In terms of usability though it's a quite a backwards change, as the new addressbar UI is clearly aiming clarify the context of what is being searched for, not to mention the inconsistency of design seeing as non-custom, regular Search engines display the title (imagine the feedback that would be caused if titles there were removed). Looking forward to seeing a new keywords management UI implemented sometime hopefully in the not too distant future, as there are many users who rely heavily on keywords and titles are a part of that just like other bookmarks and searches.

Um... I just submitted my first Mozreview review... I thought it would automatically show up here but it did not. So I'll needinfo you... It spit out two URLs. No idea what the difference is. review: https://reviewboard.mozilla.org/r/37011 (draft) review url: https://reviewboard.mozilla.org/r/37009 (draft) I tagged it r?mak but really f? is more appropriate right now, and the Mozreview docs didn't mention f?... The patch is in good shape and the review process can start but there are still some things to take care of. I ended up rewriting autocomplete-richlistitem, and I think I managed to make it simpler than it was. Other things too. One odd thing is that the title, URL, and action text is comprised of two xul:descriptions. The reason is that that was the only way I could get text-overflow: ellipsis to work. I found bug 972664, which may be relevant.

It may be related to the fact that the changeset seems to be in draft state. I'm using git lately with MozReview so I'm not sure how it could have been pushed like that.

(In reply to Drew Willcoxon :adw from comment #12) > review: https://reviewboard.mozilla.org/r/37011 (draft) > review url: https://reviewboard.mozilla.org/r/37009 (draft) I cannot access those cause they are in draft state, you must visit them and Publish the review. When you push to mozreview it asks you if you want to publish the requests, if you skip that then you must manually publish in the mozreview ui. One of the urls is the "parent" review, the other one the "child" review, cause a parent review can contain multiple changesets, each one having its review.

Based on an earlier patch by Marco Bonardo <mbonardo@mozilla.com>. Review commit: https://reviewboard.mozilla.org/r/37011/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37011/

Stephen's design spec shows only OS X chrome, so I asked him if the spec applies to all platforms, especially since we have a lot of Windows CSS that styles elements differently in different versions of Windows. Should we remove all that? > shorlander: So the tl:dr is that yes we can (should) use the same styling > across platforms. > shorlander: I think the most significant exception being that we should still > use system fonts

Note to self: The popup is always fully onscreen even if the window is partly offscreen.

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37011/diff/1-2/

The latest patch has a couple of categories of performance improvements: (1) go back to using class selectors instead of child selectors most everywhere, (2) set up item overflow (call getBoundingClientRect(), etc.) only when the item over/underflows, not every time the item is set up.

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak https://reviewboard.mozilla.org/r/37011/#review35311 It's really hard to review this, I fear the best way will be dedicating a ton of testing hours. But for that we need a patch that you consider good enough to land. I see there are some uncertain things that were never clarified with Stephen, an we likely need to clarify those. Plus Windows/Linux are not done and that makes hard to test the change cross platform. ::: browser/base/content/urlbarBindings.xml:1357 (Diff revision 2) > + this.openPopup(aElement, "after_start", 0, 6, false, false); harcoding this distance sounds dangerous, maybe we can try to calculate it, even just once and cache the result. It won't be perfect but will fail just in specific cases (like installing a theme and not restarting). ::: toolkit/components/places/UnifiedComplete.js (Diff revision 2) > -const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 "; So, looks like you are fixing bug 1054800? Please split that fix to its own bug so we can land it separately and earlier. ::: toolkit/content/autocomplete.css:15 (Diff revision 2) > -richlistitem > .ac-title-box > .ac-title > .ac-comment:not([selected]) > html|span.ac-selected-text { > +.ac-title-text, One of the big questions I had, is whether we can just change toolkit and be done, or we should override everything in browser. I still don't have that answer, but for sure doing the latter would be a nightmare. Now, I think seamonkey uses a treeview. But Thunderbird's gloda uses the richlist autocomplete, and we're likely breaking them. I'm not saying we should stop for that, but we should file a bug to let them know and see what we can do to reduce disruption (mostly ensure they can fix gloda search on top of our changes) ::: toolkit/content/autocomplete.css:19 (Diff revision 2) > + text-overflow: ellipsis; One thing I noticed when testing the patch briefly (on Windows), is that it doesn't seem to satisfy the mockup requirement regarding the space splitting and ellipsis. More specifically, page titles or the typed string, that are usually contained in the left part should never go over half of the panel. The risk is a long title will push urls really far to the right and you must visually search for them. if we limit the left part size to half, the url will always start in the first half of the panel. So title/url should be about 50/50 when a title is large. You should ask to Stephen, since I may be misremembering some details about this. ::: toolkit/content/widgets/autocomplete.xml (Diff revision 2) > - if (type == "tag" || type == "bookmark-tag") { how did you resolve the tag matching problem then? We need Stephen to tell us, I don't think we can just remove it and show results that apparently don't match anything, but in reality match a tag. ::: toolkit/content/widgets/autocomplete.xml (Diff revision 2) > - if (type == "keyword") { from a quick testing keywords look broken? (it's showing a SEARCH WITH text to me) ::: toolkit/content/widgets/autocomplete.xml:1907 (Diff revision 2) > + 4 - 16 - 16 - 4 - 16 - 4 - 4 - 32; sounds a little bit fragile? ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:17 (Diff revision 2) > +bookmarkKeywordSearch = %1$S %2$S why do we need this? can't we show the host differently? it doesn't sound like something that needs localization. Please ask stephen. Ideally we could just convert keywords to "xxx - Search with <host>" when a search string is provided. Whena search string is nt provided, we may need to do smething fancier than just showing the host...

Stephen said he'd think about tag and bookmark keyword handling. About title/URL truncation: > adw: hey stephen. re: title/url truncation, if it matters, the way i'm > handling that right now is that if the title + url combined > overflow the popup width, a max 2/3 of the width is allocated to > the title. both title and url are truncated with ellipses as > necessary > adw: here's the actual code: ... > shorlander: Yeah, that makes sense. The mockup I was going to pull was that > but the max-width for title and url was 50/50. I think the way you > are doing is better

Stephen, here's a try build (OS X only) for the work so far, in case it helps. https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-22b170a3ee1e6aaca6de1ff2e3095e84e70b0a71/

(In reply to Drew Willcoxon :adw from comment #22) > Stephen, here's a try build (OS X only) for the work so far, in case it > helps. > > https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com- > 22b170a3ee1e6aaca6de1ff2e3095e84e70b0a71/ This helps a lot. Thank you!

I filed bug 1256681 for this bug's effect on Thunderbird.

Stephen, a couple of Windows questions: There are two switch-to-tab icons, actionicon-tab-XPVista7.png and actionicon-tab.png. The former has a white version for selection but the latter doesn't. If we're now using one style across windows versions, which image should we use? For high-contrast themes and other non-default system themes, how should the text that the user typed be highlighted in results? We're limited to the tiny system color palette then (GrayText, HightlightText, etc.).

Will the issue mentioned in bug 1195054 be addressed here (i.e. having 8 results by default and getting rid of the scroll bar) or in the aforementioned bug ?

(In reply to Drew Willcoxon :adw from comment #25) > Stephen, a couple of Windows questions: > > There are two switch-to-tab icons, actionicon-tab-XPVista7.png and > actionicon-tab.png. The former has a white version for selection but the > latter doesn't. If we're now using one style across windows versions, which > image should we use? > > For high-contrast themes and other non-default system themes, how should the > text that the user typed be highlighted in results? We're limited to the > tiny system color palette then (GrayText, HightlightText, etc.). Working on some updated specs. Hopefully in time for today's meeting.

New spec: http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-02.html

So now it's 10 entries by default instead of 8 ?

Stephen, here's a new OS X build that conforms to your Update 01. https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-21fe08de684e13d6aea8bda659a44e6372f0095e/ If I could offer my opinion, the tag background gray is too dark. Distracting, makes the row seem busier. Matching the lighter gray of the icons works better IMO. We're going to need new icons for the star and tab. SVG would be great of course. The search icon is already nice and SVG but it's slightly different across platforms. It faces different directions on OS X and Windows. I presume you want to use the exact same icon everywhere now?

(In reply to Drew Willcoxon :adw from comment #30) > I presume you want to use the exact same icon everywhere now? If so we should make sure to use the same icon in the urlbar.

(In reply to Drew Willcoxon :adw from comment #30) > Stephen, here's a new OS X build that conforms to your Update 01. > > https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com- > 21fe08de684e13d6aea8bda659a44e6372f0095e/ > > If I could offer my opinion, the tag background gray is too dark. > Distracting, makes the row seem busier. Matching the lighter gray of the > icons works better IMO. Agreed. This treatment is more subtle but has the same visual effect: http://cl.ly/1X1f2J2e3O0z > We're going to need new icons for the star and tab. SVG would be great of > course. Attached to this bug. > The search icon is already nice and SVG but it's slightly different across > platforms. It faces different directions on OS X and Windows. I presume > you want to use the exact same icon everywhere now? We observe a platform specific difference with the way the search icon is flipped, so we can keep using the platform specific autocomplete-search.svg.

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37011/diff/2-3/

That's the "final" for-review patch -- not including whatever comments you have for it of course. Addressing your previous comments: (In reply to Marco Bonardo [::mak] from comment #20) > ::: browser/base/content/urlbarBindings.xml:1357 > (Diff revision 2) > > + this.openPopup(aElement, "after_start", 0, 6, false, false); > > harcoding this distance sounds dangerous, maybe we can try to calculate it, > even just once and cache the result. It won't be perfect but will fail just > in specific cases (like installing a theme and not restarting). This is now in the browser.css for each platform, like: #PopupAutoCompleteRichResult { margin-top: 8px; } > ::: toolkit/components/places/UnifiedComplete.js > (Diff revision 2) > > -const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 "; > > So, looks like you are fixing bug 1054800? Please split that fix to its own > bug so we can land it separately and earlier. I don't want to go back and untangle that now, so I left this as is. Especially considering that that bug is not a high priority at all. > ::: toolkit/content/widgets/autocomplete.xml:1907 > (Diff revision 2) > > + 4 - 16 - 16 - 4 - 16 - 4 - 4 - 32; > > sounds a little bit fragile? Yeah, I calculate this now by subtracting the title rect's left from the item width. > ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:17 > (Diff revision 2) > > +bookmarkKeywordSearch = %1$S %2$S > > why do we need this? can't we show the host differently? it doesn't sound > like something that needs localization. This has a colon in it now, but to address your comment, before this patch we got RTL behavior here because the domain and keyword arg part were different boxes. Now they're one box, so we need an l10n string to accommodate RTL languages.

Stephen, would you mind trying this out and letting me know any changes you might have? Would be great if you could test on all three platforms. I think this should match your spec, but I might have missed something. https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-1b78709b68e34318ea103732ac1bda6dc6ab41e5/

I've tried the test build on Windows. It looks really nice. A few questions though: -Would it be possible to have the "action" icons (search lens, globe, etc.) invert on hover ? They are barely visible with the blue hover effect (note that in current implementation these icons minus the globe invert on hover). -There seems to be more duplication in entries. Example when I type "in" it autocompletes to inbox.google.com and it proposes the website at least 4 times (autocomplete, bookmark, history, switch to tab). It isn't optimal in current implementation either as the same site is proposed 2 times (autocomplete and bookmark), but it's still better. -With the smaller entries it would be nice having more of them shown by default and at the same time finally getting rid of this useless scroll bar. If I find other bugs I mention them.

(In reply to Guillaume C. [:ge3k0s] from comment #39) > -With the smaller entries it would be nice having more of them shown by > default and at the same time finally getting rid of this useless scroll bar. It will happen in a followup. we didn't decide whether we'll go to 8 or 10 results (I'd vote to start with 10 and then use telemetry to tweak it.)

(In reply to Marco Bonardo [::mak] from comment #40) > (In reply to Guillaume C. [:ge3k0s] from comment #39) > > -With the smaller entries it would be nice having more of them shown by > > default and at the same time finally getting rid of this useless scroll bar. > > It will happen in a followup. we didn't decide whether we'll go to 8 or 10 > results (I'd vote to start with 10 and then use telemetry to tweak it.) Yes, I think 10 would be a good place to start. Do we have a bug for this already? I can file one if not.

(In reply to Drew Willcoxon :adw from comment #38) > Stephen, would you mind trying this out and letting me know any changes you > might have? Would be great if you could test on all three platforms. I > think this should match your spec, but I might have missed something. > > https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com- > 1b78709b68e34318ea103732ac1bda6dc6ab41e5/ Most everything looks good to me! Although I wasn't able to test on Linux, build wouldn't run for some reason. (In reply to Guillaume C. [:ge3k0s] from comment #39) > -Would it be possible to have the "action" icons (search lens, globe, etc.) > invert on hover ? They are barely visible with the blue hover effect (note > that in current implementation these icons minus the globe invert on hover). Yeah, for some reason the search icon doesn't get inverted on Windows, but does on OS X. Also it would be good to invert the default favicon if we can. Do we have an inverted version?

https://reviewboard.mozilla.org/r/37011/#review38833 ::: browser/themes/windows/browser.css:1568 (Diff revision 3) > + list-style-image: url(chrome://global/skin/icons/autocomplete-search.svg#search-icon); > +} > + > +.ac-type-icon[type=keyword][selected], > +.ac-site-icon[type=searchengine][selected], > +.autocomplete-treebody::-moz-tree-image(keyword, treecolAutoCompleteImage, selected), Syntax error: this line should end with a { not a comma. That prevents keyword and searchengine results from having their magnifying glass icon inverted when selected. Fixed locally.

https://reviewboard.mozilla.org/r/37011/#review38843 ::: browser/themes/linux/browser.css:1097 (Diff revision 3) > display: block; > } > > +/* overlap the urlbar's border */ > +#PopupAutoCompleteRichResult { > + margin-top: 8px; This should be 9px, fixed locally.

(In reply to Stephen Horlander [:shorlander] from comment #42) > Most everything looks good to me! Although I wasn't able to test on Linux, > build wouldn't run for some reason. Hmm, dumb questions: Did you use this one? https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com-1b78709b68e34318ea103732ac1bda6dc6ab41e5/try-linux64/firefox-47.0a1.en-US.linux-x86_64.tar.bz2 Did you run firefox-bin? > Yeah, for some reason the search icon doesn't get inverted on Windows Thanks, I've fixed that locally. (It's due to a syntax error in the Windows CSS.) > Also it would be good to invert the default favicon if we can. > Do we have an inverted version? We don't have an inverted version. That icon lives here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/mozapps/places/ I'm pretty sure (not totally sure) that in the case of awesomebar popup entries, this icon comes from the Places favicon service's default favicon, and it's hooked up in the back-end in UnifiedComplete. It may be a little tricky to choose a different icon based on whether a particular result is highlighted, but I think we should definitely give it a shot. I think we should do that in a separate bug though and not have it block this one from landing. I'll file a new bug and CC you, so maybe you could attach a new invertable SVG to it.

(In reply to Stephen Horlander [:shorlander] from comment #41) > Yes, I think 10 would be a good place to start. Do we have a bug for this > already? I can file one if not. Filed bug 1259575. Marked it as a see-also for this one since it doesn't exactly depend on this or vice versa.

Ah, I think I grabbed the wrong Linux build. That one worked. So, looks good but the panel is dark on Ubuntu. I don't think that used to be the case. http://cl.ly/1J333e22063L

Hmm, I've been testing on Fedora. I'll try Ubuntu.

Might be picking up the context menu styling. But it used to look like this: http://cl.ly/0L0T2e43213g

(In reply to Stephen Horlander [:shorlander] from comment #42) > (In reply to Drew Willcoxon :adw from comment #38) > > Stephen, would you mind trying this out and letting me know any changes you > > might have? Would be great if you could test on all three platforms. I > > think this should match your spec, but I might have missed something. > > > > https://archive.mozilla.org/pub/firefox/try-builds/dwillcoxon@mozilla.com- > > 1b78709b68e34318ea103732ac1bda6dc6ab41e5/ > > Most everything looks good to me! Although I wasn't able to test on Linux, > build wouldn't run for some reason. > > (In reply to Guillaume C. [:ge3k0s] from comment #39) > > -Would it be possible to have the "action" icons (search lens, globe, etc.) > > invert on hover ? They are barely visible with the blue hover effect (note > > that in current implementation these icons minus the globe invert on hover). > > Yeah, for some reason the search icon doesn't get inverted on Windows, but > does on OS X. Also it would be good to invert the default favicon if we can. > Do we have an inverted version? In my testing the switch to tab and bookmark favicons didn't invert either.

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37011/diff/3-4/

There were two test failures on try, and the latest patch fixes both for me locally. The xpcshell failure was due to setting match.comment = match.engineName instead of parseResult.engineName in UnifiedComplete, and the mochitest failure was due to not resetting this.selectedIndex = -1 in urlbar-rich-result-popup's overridden _openAutocompletePopup. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5176629e3a

(In reply to Stephen Horlander [:shorlander] from comment #49) > Might be picking up the context menu styling. But it used to look like this: > http://cl.ly/0L0T2e43213g There's something going on on Unbuntu where the awesomebar popup is brown on Nightly builds but white on release builds. I can't figure it out. The browser toolbox shows the same rules for each. So this doesn't appear to be due to the patch.

It's not just the awesomebar popup either, it's also the search bar popup. So I'm guessing it has something to do with how Gecko draws -moz-appearance:menupopup or display:-moz-popup.

But with the new design we're now hardcoding text colors -- but not the popup background color! -- which looks bad on the brown background on Nightly. So we need to fix that somehow. Options: (1) Continue to use "system" colors for text on Linux (GrayText, etc.), but since there are a small number of these, it's hard if not impossible to do the emphasis coloring called for by the spec. (2) Maybe we could add new system colors to Gecko specifically for this use case. (3) Force the popup background color to be white on Linux, i.e., stop using -moz-appearance:menupopup or whatever and give it a background-color. Then we control both the text colors and popup color. (Do that for all platforms? It must be a happy coincidence that Gecko draws Windows and OS X popups with a white background?)

(4) We could add an %ifndef RELEASE_BUILD or something like that in the Linux CSS and use system colors only in that case. That's too Ubuntu-specific IMO though. Maybe there's a media query similar to the queries for non-default Windows themes we could use?

(In reply to Drew Willcoxon :adw from comment #53) > There's something going on on Unbuntu where the awesomebar popup is brown on > Nightly builds but white on release builds. Release builds use gtk2, whereas all other branches use gtk3 IIRC. If you see a brown popup in aurora and beta, then that may be what is causing it.

Thanks Panos. Bug 1238866 has a similar problem. Mike says that 46 uses Gtk+3, while earlier versions use Gtk+2. That would explain why the popup is brown only on Nightly right now, I guess.

Some things I noticed while testing: 1. bookmark keywords with a search term show both the magnifier icon and the default world favicon... The mockup shows the same (but with a favicon, that I suspect in most cases we won't have). Any reason for not showing only the magnifier? 2. when we match part of a tag, the matched part is grey on grey, not much readable. What can we do about that? Would it be bad to only use a grey border and avoid the backfill? or change the emphasis effect on tags... 3. In general I'm not a big fan of this light emphasis on Windows. It looks like a graphical glitch where some part of the popup is discolored, likely cause the font is thinner than on OSX. The text "disappears" into the light background. Moreover the system browsers do the opposite: IE emphasizes the text with light blue, Edge uses bold, so they make the match MORE evident. Chrome uses bold too, but differently depending on text being suggested or not, though with a larger font. Overall the new emphasis design doesn't seem very accessible to people having visual problems. 4. Can we avoid the popup dropshadow? Not sure if this is feasible today, worth asking Enn? All of these are things I'd like us to go through, but they are not show stoppers

Dunno if this helps understanding what I mean by emphasis looking bad in my case.

Those are all questions for Stephen and I'd needinfo him but he's already needinfoed. :-) (In reply to Marco Bonardo [::mak] from comment #59) > 4. Can we avoid the popup dropshadow? Not sure if this is feasible today, > worth asking Enn? In your screenshot, the part of the shadow on the right edge of the popup doesn't look good, I agree. Not sure whether I agree that the bottom edge doesn't want a shadow though -- maybe. Adding a return NS_STYLE_WINDOW_SHADOW_NONE in nsMenuPopupFrame::GetShadowStyle makes the shadow go away. Seems like we should be able to add a new attribute on panel if we want. But then the popup footer (with the "Change Search Settings" button) has no bottom border -- probably fixable in CSS. I missed Neil on IRC today but I can ask him tomorrow. I also noticed in your screenshot that the panel's position needs to be moved down a little so that it doesn't overlap the navbar at all. I've been testing on Windows 7, where the panel is positioned correctly. I haven't tested on Windows 10, I should probably get a copy. I wonder whether there's a way to get the correct y-coord programmatically without having to hardcode a margin-top per platform and Windows version?

(In reply to Drew Willcoxon :adw from comment #61) > In your screenshot, the part of the shadow on the right edge of the popup > doesn't look good, I agree. Not sure whether I agree that the bottom edge > doesn't want a shadow though -- maybe. Yeah I don't care much about the bottom edge, I said to remove the drop shadow cause the mock-up shows a simple line. > I wonder whether > there's a way to get the correct y-coord programmatically without having to > hardcode a margin-top per platform and Windows version? I didn't try, can we can get the bounding rect of the nav-bar? If you need help fixing win 10, just let me know.

re: drop shadow, see bug 118379

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak https://reviewboard.mozilla.org/r/37011/#review40091 OK, apart from the previous feedback, I only have one major complain that is the fact we hardcode most colors makes this not very nice to color blind users, moreover this won't obey hi-contrast theming and that's a major problem for who really needs that. Maybe we should provide a fallback for the high contrast theme. ::: browser/base/content/urlbarBindings.xml:1346 (Diff revision 4) > + return; > + } > + > + this.selectedIndex = -1; > + > + this.mInput = aInput; nit: set mInput before selectedIndex, as in the original method this overrides. currently there's no reason, but selectedIndex is a setter and could eventually use mInput. ::: toolkit/content/widgets/autocomplete.xml:1636 (Diff revision 4) > + > + let anyTagsMatch = false; > + for (let tag of tags) { > + let search = this.getAttribute("text"); > + let tokens = this._getSearchTokens(search); > + let indices = this._getBoundaryIndices(tag, tokens); Please add a brief comment like // Check if the search token matches this tag. At first I had difficulties reading this code cause I was trying to figure out why you were using indices, unless I figured that's what it was doing. ::: toolkit/content/widgets/autocomplete.xml:2007 (Diff revision 4) > + // Total width for the title and URL/action is the width of the item > + // minus the start of the title text minus a little extra padding. > + let itemWidth = > + this.parentNode.getBoundingClientRect().width - > + this._titleText.getBoundingClientRect().left - > + 30; how is this extra padding calculated? The comment doesn't seem to help figuring that out, nor whether it should be kept in sync with something in the css files Maybe it's also worth to move it to a commented const ::: toolkit/content/widgets/autocomplete.xml:2017 (Diff revision 4) > + } > + > + let titleTagsWidth = titleRect.width + tagsRect.width; > + if (titleTagsWidth + urlActionWidth > itemWidth) { > + // Title + tags + URL/action overflows the item width. > + let titleTagsPct = 0.66; please document the meaning of this percentage ::: toolkit/content/widgets/autocomplete.xml:2025 (Diff revision 4) > + titleTagsAvailable, > + itemWidth * titleTagsPct > + ); > + if (titleTagsWidth > titleTagsMaxWidth) { > + // Title + tags overflows the max title + tags width. > + let titlePct = 0.33; please document the meaning of this percentage ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:10 (Diff revision 4) > # LOCALIZATION NOTE (searchWithEngine): %S will be replaced with > # the search engine provider's name. This format was chosen because > # the provider can also end with "Search" (e.g.: MSN Search). > searchWithEngine = Search with %S > -switchToTab = Switch to tab > -# LOCALIZATION NOTE (visitURL): > + > +switchToTab2 = Switch to Tab Why are you changing the name? the string is the same, I don't expect anyone to figure out what changed if you don't add a localization note... ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:11 (Diff revision 4) > # the search engine provider's name. This format was chosen because > # the provider can also end with "Search" (e.g.: MSN Search). > searchWithEngine = Search with %S > -switchToTab = Switch to tab > -# LOCALIZATION NOTE (visitURL): > -# %S is the URL to visit. > + > +switchToTab2 = Switch to Tab > +visitURL2 = Visit this could also just be renamed to visit... but it also calls for a localization note to explain how it's being used. ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:17 (Diff revision 4) > -visitURL = Visit %S > + > +# LOCALIZATION NOTE (bookmarkKeywordSearch): This is the title of autocomplete > +# entries that are bookmark keyword searches. %1$S will be replaced with the > +# domain name of the bookmark, and %2$S will be replaced with the keyword > +# search text that the user is typing. %2$S will not be empty. > +bookmarkKeywordSearch = %1$S: %2$S I honestly think we should reuse searchWithEngine for keywords and have it read Search with <domain>... In the end we plan to make keywords custom engines... That said, it's clear we could do that when it will happen. ::: toolkit/themes/linux/global/autocomplete.css:108 (Diff revision 4) > > .autocomplete-richlistbox { > -moz-appearance: none; > margin: 1px; > background-color: transparent; > + padding: 4px; any reason this should be here rather than in the browser theme? (for all themes)

Thanks Marco. I don't think we should land this until we decide what to do for Ubuntu. I don't want to get swamped with complaints (rightfully) from all the people on the platform team who use Linux. (In reply to Marco Bonardo [::mak] from comment #64) > OK, apart from the previous feedback, I only have one major complain that is > the fact we hardcode most colors makes this not very nice to color blind > users, moreover this won't obey hi-contrast theming and that's a major > problem for who really needs that. Maybe we should provide a fallback for > the high contrast theme. The Windows CSS falls back to system colors for non-default themes. Which is actually too bad for non-default but non-high-contrast themes (like the classic theme) IMO. It would be great to have a better solution for those themes. I don't know how relevant that is to Windows 10 though -- is it even possible to switch themes on 10? > ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:10 > (Diff revision 4) > > # LOCALIZATION NOTE (searchWithEngine): %S will be replaced with > > # the search engine provider's name. This format was chosen because > > # the provider can also end with "Search" (e.g.: MSN Search). > > searchWithEngine = Search with %S > > -switchToTab = Switch to tab > > -# LOCALIZATION NOTE (visitURL): > > + > > +switchToTab2 = Switch to Tab > > Why are you changing the name? the string is the same, I don't expect anyone > to figure out what changed if you don't add a localization note... The string actually isn't the same. It now uses title case, so Tab is now capitalized. I don't know for sure whether capitalization changes warrant string name changes but I don't see how they could not since uppercase vs. lowercase is a thing in many languages. I'll add a note though. > ::: toolkit/themes/linux/global/autocomplete.css:108 > (Diff revision 4) > > > > .autocomplete-richlistbox { > > -moz-appearance: none; > > margin: 1px; > > background-color: transparent; > > + padding: 4px; > > any reason this should be here rather than in the browser theme? (for all > themes) No, to be honest it's not really clear to me at all what should go in toolkit and what should go in browser. I'll move it to browser.

(In reply to Drew Willcoxon :adw from comment #65) > The Windows CSS falls back to system colors for non-default themes. Which > is actually too bad for non-default but non-high-contrast themes (like the > classic theme) IMO. It would be great to have a better solution for those > themes. I don't know how relevant that is to Windows 10 though -- is it > even possible to switch themes on 10? Hi contrast themes are available. I honestly didn't test the patch with it though, to check how we are behaving. I can do that. > > ::: toolkit/locales/en-US/chrome/global/autocomplete.properties:10 > > > +switchToTab2 = Switch to Tab > > > > Why are you changing the name? the string is the same, I don't expect anyone > > to figure out what changed if you don't add a localization note... > > The string actually isn't the same. It now uses title case, so Tab is now > capitalized. I don't know for sure whether capitalization changes warrant > string name changes but I don't see how they could not since uppercase vs. > lowercase is a thing in many languages. I'll add a note though. yeah, if I didn't notice the subtle difference, I can imagine many won't as well. > > ::: toolkit/themes/linux/global/autocomplete.css:108 > No, to be honest it's not really clear to me at all what should go in > toolkit and what should go in browser. I'll move it to browser. Well, this padding is non-standard, and as such I'd expect it to be in browser. toolkit should likely have the standard style for the given platform, unless the style is actually needed for the widget to work properly (and doesn't look the case)

I forgot to ask, did you test the dev-edition dark theme by chance?

(In reply to Marco Bonardo [::mak] from comment #66) > Hi contrast themes are available. > I honestly didn't test the patch with it though, to check how we are > behaving. I can do that. That would be nice, thanks. I tested on Windows 7 and it looks OK, but as I said there's no emphasis since that's done with text colors. (In reply to Marco Bonardo [::mak] from comment #67) > I forgot to ask, did you test the dev-edition dark theme by chance? Not yet, but I'll do that. Is there an easier way than building Aurora?

(In reply to Drew Willcoxon :adw from comment #68) > (In reply to Marco Bonardo [::mak] from comment #67) > > I forgot to ask, did you test the dev-edition dark theme by chance? > > Not yet, but I'll do that. Is there an easier way than building Aurora? I think you can just switch themes in your custom build. The dev-edition theme is present in those.

(In reply to Marco Bonardo [::mak] from comment #59) > Some things I noticed while testing: > 1. bookmark keywords with a search term show both the magnifier icon and the > default world favicon... The mockup shows the same (but with a favicon, that > I suspect in most cases we won't have). Any reason for not showing only the > magnifier? The rationale for this is that it would be nice to visually link the site associated with the keyword to the search. Particularly to disassociate bookmark keyword searches from default search engine searches. I don't know of a way to verify whether or not we won't have the favicon in most cases. > 2. when we match part of a tag, the matched part is grey on grey, not much > readable. What can we do about that? Would it be bad to only use a grey > border and avoid the backfill? or change the emphasis effect on tags... I can try some different styling options for this. > 3. In general I'm not a big fan of this light emphasis on Windows. It looks > like a graphical glitch where some part of the popup is discolored, likely > cause the font is thinner than on OSX. > The text "disappears" into the light background. Moreover the system > browsers do the opposite: IE emphasizes the text with light blue, Edge uses > bold, so they make the match MORE evident. Chrome uses bold too, but > differently depending on text being suggested or not, though with a larger > font. Overall the new emphasis design doesn't seem very accessible to people > having visual problems. The rationale here is that you know what you are searching for since you just typed it, what you really care about is context of where that term is found. We also used to do bolding, but I would prefer to avoid it if possible because it causes a lot of text shifting. We can monitor feedback and keep an eye on it though. > 4. Can we avoid the popup dropshadow? Not sure if this is feasible today, > worth asking Enn? Are you saying we should lose the dropshadow entirely?

Comment on attachment 8724861 [details] MozReview Request: Bug 1181078 - Implement new awesomebar design. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37011/diff/4-5/

That's the final patch. I pushed it to reviewboard (and carried forward the r+) to show the changes between it and the r+'ed patch. The changes are: * Use system colors for text on Linux. I talked with Stephen about it and that's what we decided to do for now. * Improved coloring of tags where system colors are used (Windows non-default themes and Linux): use MenuText as the background color and Menu as the text color. Previously tags were not colored differently from title text. * Tested on dev edition, adjusted padding slightly per platform to account for changes vs. the normal theme. * Addressed comments in comment 64. * Uses bounding rects of navbar and urlbar to open the popup in the correct y position.

Hmm, that reviewboard interdiff is apparently showing unrelated changes due to refreshing my tree since the previous reviewboard commit. Making it hard to see what actually changed. Maybe because I'm using mqueues instead of bookmarks or something.

One final try push before landing. I don't expect any problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeddab7ccbf1

Missed a chrome test, which also caused failures in two subsequent tests. New try push with fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f4b5fe87ca

What sort of testing does this need? Should it have had a test plan before landing?

(In reply to Drew Willcoxon :adw from comment #65) > The string actually isn't the same. It now uses title case, so Tab is now > capitalized. I don't know for sure whether capitalization changes warrant > string name changes but I don't see how they could not since uppercase vs. > lowercase is a thing in many languages. I'll add a note though. For future reference: each language should have its own rules for capitalization, so this kind of changes doesn't require a new string ID https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

(In reply to Ryan VanderMeulen [:RyanVM] from comment #78) > What sort of testing does this need? Should it have had a test plan before > landing? This is mostly a visual change and was tested internally by most of our team (at least Drew, me, Stephen and Panos). I agree it needs some good testing cause it's the awesomebar, but it also has the whole rapid release timeframe to do that. Though, I think next time we should indeed involve QA earlier in the change, not doing that was a mistake. We clearly would appreciate much some smoke, accessibility and cross-platform (themes/dpi) testing.

(In reply to Stephen Horlander [:shorlander] from comment #70) > The rationale for this is that it would be nice to visually link the site > associated with the keyword to the search. Particularly to disassociate > bookmark keyword searches from default search engine searches. why should we? The long term plan is to make keywords a custom search engine. They should be manageable from Preferences /Search as any other search engine keyword. > I don't know of a way to verify whether or not we won't have the favicon in most cases. The code doesn't set an icon for keywords, The only thing we may do is trying to guess "domain/favicon.ico" and hope it's in the db... Would that suffice? Though it still seems we don't really need to differentiate from search engines... > > 2. when we match part of a tag, the matched part is grey on grey, not much > > readable. What can we do about that? Would it be bad to only use a grey > > border and avoid the backfill? or change the emphasis effect on tags... > > I can try some different styling options for this. I guess we need a dependency bug then. I will file one. > The rationale here is that you know what you are searching for since you > just typed it, what you really care about is context of where that term is > found. Yes, I figured that already, it still looks quite bad imo (dunno if you saw the sshot I posted). > > 4. Can we avoid the popup dropshadow? Not sure if this is feasible today, > > worth asking Enn? > > Are you saying we should lose the dropshadow entirely? Very likely cause I don't think we can tell the OS to draw only a part of it. At a maximum we should have one at the bottom. The non maximized window looks bad. I will file a dependency and we can evaluate there.

(In reply to Marco Bonardo [::mak] from comment #80) > Though, I think next time we should indeed involve QA earlier in the change, > not doing that was a mistake. > We clearly would appreciate much some smoke, accessibility and > cross-platform (themes/dpi) testing. Yeah, we're trying to get more deliberate about new feature work and refactorings not riding the trains without a QA pass first. I'll have my team get in touch with you.

I'm running on inbound and noticed that a patch changed the size of the awesome bar drop down list. Before it was as wide as the length of the awesome bar, now it's the full width of the Fx window. Just wondering if this patch is the cause and if it's intentional?

I ran moz regression and came up with this pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5448e34dc116967151f3f6a0271d8ae4a2304796&tochange=49d808d13f4fab71b49019905cce3ba6eb6bb31f

(In reply to Gary [:streetwolf] from comment #84) > Just wondering if > this patch is the cause and if it's intentional? Yes, it's intentional, as you can see from the mock-up in the URL of this bug.

Is there a hidden pref, or just any pref to turn this off - it does not look good, the list is too short, and the bright-blue marker when scrolling the list it eye-jarring.

I filed bug 1262507 to collect follow-up and polish issues, and it may also be useful to track regression bugs (blocking a closed bug is not particularly useful to track progress). Please use it.

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #87) > Is there a hidden pref, or just any pref to turn this off The short list will be fixed, we will move to a 10 items list by default. I'm about to make bug 1195054 track that change. No prefs. If there are issues please file them and make them depend on this bug and block bug 1262507. Thank you for the help!

Confirmation: SV-LA will develop a test plan

Test Results can be seen here https://docs.google.com/spreadsheets/d/1x5L9Od7fCgEE6xvJ_4INNRARMVjaGyry9lHpieIFZBs/edit#gid=2079057953 Awesome Bar (URL/Search) meets expectations; we visually see textual distinction of type text and match results in dropdown Favicons/Maginifer/Synced Tab/Favorite icons display URLs and Links are displayed and functional Icons to the left and right of the Awesome Bar are still displayed and function as expected Will send an official notice.

I think it's worth a relnote for this. Release Note Request (optional, but appreciated) [Why is this notable]: Much more modern and polished awesomebar! [Suggested wording]: Something along the lines "The awesomebar has been redesigned with a modern styling and now it shows even more useful results at once" [Links (documentation, blog post, etc)]: So far, we only have the design mock-up, I'm not sure it'd be really useful to final users: http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-02.html I'm not great with wording, any suggestions?

(In reply to Marco Bonardo [::mak] from comment #93) > [Suggested wording]: Something along the lines "The awesomebar has been > redesigned with a modern styling and now it shows even more useful results > at once" Another idea: "The URL bar result panel has a new design and shows more results" I think we don't use the awesomebar moniker in user-facing communications.

Marking this Verified since the new awesomebar landed into Firefox 48 Beta. The remaining issues will be tracked separately.

Hi. I would like to report a bug. It is the new awfulbar (which some unaccountably seem to call the "awesomebar"). This bug results in: (i) frequent duplication (due to the fact that the top, superfluous, and widely-derided 'Visit' line frequently duplicating the next URL down). (ii) frequently unhelpful 'search' results, due to an overemphasis on visit-frequency (typing in a single letter should NOT result in a domain-name which has the letter as its fifth letter before another less-frequently-visited domain which has it as its first). (iii)difficulty to read, due to over-promiscuous bolding. I would note that the old awful bar also created considerable negative comment, but at least it could be disabled with an about:config switch. I am sure that if anybody ever bothered to read various forums on the subject, you would find similar, and further, complaints.

this is not the right way to report issues and most of those are already files. Please look for the proper bugs, and open new one (one per issue) where the are not yet tracked. also, please try to avoid advocacy pitfalls, this is a ticketing system, not a discussion forum.