Closed Bug 1019383 Opened 6 years ago Closed 6 years ago

Implement search-engine discovery notification. Part 1: add API

Categories

(SeaMonkey :: Search, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: pjdkrunkt, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26 (Beta/Release)
Build ID: 20140428215944

Steps to reproduce:

This is a followup to Bug 963132

In the old days, Firefox used to have a visual notification on the .searchbar-engine-button that Open Search engines were available to install.  It would make the search-engine-button glow aqua blue.  (Not sure if this was ever in SeaMonkey?)  

The support to hook this up is pretty simple.  I adapted this from my Luddite UI extension, and I had originally adapted it from Firefox 3.6.  Every time you update the search-engine status, you update the searchbar-engine-button as well:

if (document.getElementById("searchbar")) {
  if (getBrowser().mCurrentBrowser.engines && getBrowser().mCurrentBrowser.engines.length > 0) {
    document.getElementById("searchbar").searchButton.setAttribute("addengines", "true");
  } else {
    document.getElementById("searchbar").searchButton.removeAttribute("addengines");
  }
}

And some CSS for proof of concept:

.searchbar-engine-button[addengines="true"] {
  background-color:#BDE0EB!important;
}
Attached patch Patch v1.0 Proposed Fix. (obsolete) — Splinter Review
> -    if (getBrowser().shouldLoadFavIcon(targetDoc.documentURIObject))

The trouble with shouldLoadFavIcon() is that it depends on browser.chrome.site_icons and browser.chrome.favicons

> -      iconURL = getBrowser().buildFavIconString(targetDoc.documentURIObject);
> +    var aURI = targetDoc.documentURIObject;
> +    try {
> +      aURI = Services.uriFixup.createExposableURI(aURI);
> +    } catch (e) {
> +    }
> +
> +    if (aURI && ("schemeIs" in aURI) &&
> +        (aURI.schemeIs("http") || aURI.schemeIs("https")))
> +      iconURL = getBrowser().buildFavIconString(aURI);

> +.searchbar-engine-button[addengines="true"]:not([open="true"]) {
> +  background-color: #BDE0EB;
Using Patrick's aqua blue colour.
Assignee: nobody → philip.chee
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8436036 - Flags: review?(neil)
Attachment #8436036 - Flags: feedback?(pjdkrunkt)
Comment on attachment 8436036 [details] [diff] [review]
Patch v1.0 Proposed Fix.

> function pageShowEventHandlers(event)
> {
>   checkForDirectoryListing();
>+  BrowserSearch.updateSearchButton();
Doesn't the call in nsBrowserStatusHandler suffice?

>+.searchbar-engine-button[addengines="true"]:not([open="true"]) {
>+  background-color: #BDE0EB;
>+}
I'm not convinced by this colour, particularly in Modern, where it doesn't touch the border.
How about highlighting the dropmarker as if the menu was open?
(I don't know what to do on the Mac though.)
Comment on attachment 8436036 [details] [diff] [review]
Patch v1.0 Proposed Fix.

>> function pageShowEventHandlers(event)
>> {
>>   checkForDirectoryListing();
>>+  BrowserSearch.updateSearchButton();
> Doesn't the call in nsBrowserStatusHandler suffice?

I'll check.

>>+.searchbar-engine-button[addengines="true"]:not([open="true"]) {
>>+  background-color: #BDE0EB;
>>+}
> I'm not convinced by this colour, particularly in Modern, where it doesn't
> touch the border.
> How about highlighting the dropmarker as if the menu was open?
Should be doable.

> (I don't know what to do on the Mac though.)
Let's ask Stefanh.
Attachment #8436036 - Flags: review?(neil)
Attachment #8436036 - Flags: feedback?(pjdkrunkt)
Sorry, we don't have any button here, just the dropmarker so this will look a bit weird imo (see screenshot). It might work changing/highlighting the drop-down (not sure, though). Note also that the drop-down doesn't highlight on mac when the menu opens.

Btw, this sort of introduces a new "notification" (I'm not sure that users will get what it means). Wouldn't it be better using the existing notification system?
I could add a doorhanger anchored to the search bar, what do you guys think?
Do you really want a doorhanger or a notification panel to pop up on every single site that offers a search plugin?  I always viewed it as something more like an RSS feed... yes you want a notification that it's available, but you don't want it to be an annoyance.
(In reply to patrickjdempsey from comment #6)
> Do you really want a doorhanger or a notification panel to pop up on every
> single site that offers a search plugin?  I always viewed it as something
> more like an RSS feed... yes you want a notification that it's available,
> but you don't want it to be an annoyance.

I thought about that. The doorhanger or notification should have a "Don't show this again ever" option. Increases discoverability.

I'm also thinking of splitting off the UI into another bug. This one will be just adding or removing the "addengines" attribute. This way themes or extensions can make use of it.
>> function pageShowEventHandlers(event)
>> {
>>   checkForDirectoryListing();
>>+  BrowserSearch.updateSearchButton();
>> Doesn't the call in nsBrowserStatusHandler suffice?
Yes it suffices. Removed.

>>+.searchbar-engine-button[addengines="true"]:not([open="true"]) {
>>+  background-color: #BDE0EB;
>>+}
> I'm not convinced by this colour, particularly in Modern, where it doesn't touch the border.
> How about highlighting the dropmarker as if the menu was open?
> (I don't know what to do on the Mac though.)
I've removed all the CSS/UI. I'll leave the bikeshedding for another bug. Meanwhile theme authors can make use of the "addengines" attribute.
Attachment #8436036 - Attachment is obsolete: true
Attachment #8454443 - Flags: review?(neil)
Attachment #8454443 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/89ff1e578134
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Implement search-engine discovery notification → Implement search-engine discovery notification. Part 1: add API
Target Milestone: --- → seamonkey2.30
Blocks: 1107088
You need to log in before you can comment on or make changes to this bug.