402895 - The search bar no longer has a blue border when the website has a search engine available

Status: VERIFIED FIXED

(Firefox :: Search, defect, P4)

Description

[Frédéric Buclin]

Attachment: screenshot

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110618 Firefox/3.0b1

Go visit http://technorati.com/ or http://fr.wikipedia.org/wiki/Accueil and look at the search bar. The border is not in blue despite a search engine is available and I don't have it installed yet.

This is definitely a regression as I can see the blue border with Firefox 2.0.0.9.
I'm running Mandriva Linux 2008.0 with KDE 3.5.7, and yes, I'm using a new profile (I deleted .mozilla/ to be sure) and no, I have no plugin/addon installed.

Comment 1

[Kevin Brosnan]

This works for me on b.m.o and the sites mentioned in comment 0.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110704 Minefield/3.0a9pre

I'll give the beta rc a check.

Comment 2

[Frédéric Buclin]

Hum, weird, I cannot reproduce the problem after I restarted Fx3. Maybe the problem was due to some action I did previously. Didn't find a testcase yet -> minor.

Comment 3

[Frédéric Buclin]

Ah ok, testcase found:

on the navigation toolbar, right click and select "Customize", then add or remove a button (in my case, the printer), then close the "Customize toolbar" window. Next time you try to access a website with a search engine, the search bar will no longer appear in blue.

Comment 4

[Marcia Knous [:marcia]]

I see this on Mac as well, using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b1) Gecko/2007110618 Firefox/3.0b1, following the steps in Comment 3.

Comment 5

[Kevin Brosnan]

For me just opening and closing the customize window causes this. With the build in comment 1.

Comment 6

[Frédéric Buclin]

Marcia can reproduce on Mac -> All/All.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Strange, for some reason the style rules in searchBar.css no longer apply after the toolbar customzation code "wraps" the nodes in the toolbar, even though the attribute is still set correctly.

A regression range would be useful here, this doesn't appear to be related to any front-end search changes.

Comment 8

[Dão Gottwald [:dao]]

Attachment: fix

After customizing the toolbar, BrowserSearch.searchBar.searchButton still points to the old element. I don't know why.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

That sounds broken. We should try to figure out the root cause before wallpapering over it like this, I think.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(Although we can always take that patch if we can't figure it out soon enough - but even then we should get another bug filed.)

Comment 11

[Dão Gottwald [:dao]]

It looks like an XBL issue to me, but then I would expect it to occur in more places, which doesn't seem to be the case. At this point I don't see what's special about the searchButton property.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Wait, I'm not sure I understand - I can reproduce by just opening and closing the customize toolbar dialog - what's the "old element" in that case? We wrap and unwrap the button, but we don't touch the button itself afaik.

Comment 13

[Dão Gottwald [:dao]]

Both wrapping and unwrapping the search bar reconstructs the binding, including the anonymous content (e.g. the button) as it seems. But the _searchButton reference persists.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Oh, I see. _searchButton is persisting when the binding is re-attached (with new anonymous content) because it's just a normal JS property. I suppose we could fix this by creating a field for it (initialized to null). Or even just use a field instead of a property now that fields are lazily evaluated, although then we'd lose the ability to make it read-only. Having it be a field would ensure that it gets re-initialized when the binding is re-applied, I think.

(I'm also not sure why the binding is being reapplied and anonymous content being rebuilt, but I suppose that's something we can look into separately.)

Comment 15

[Dão Gottwald [:dao]]

I tried adding a _searchButton field and using a field for searchButton ... didn't make a difference.

Comment 16

[Dão Gottwald [:dao]]

Jonas, Boris, any thoughts?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Using a field should be working.  See bug 398135: we undefine fields when we tear down the binding, so they will get redefined again later.

> I'm also not sure why the binding is being reapplied

Are you moving the node around in the DOM?  If you are, that's why.

Comment 18

[Dão Gottwald [:dao]]

Attachment: v2

OK, this appears to be working. Don't know what went wrong last time.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 290428 [details] [diff] [review]
v2

Loses the read-only-ness, but I guess that's OK.

Comment 20

[Reed Loden [:reed]]

Checking in browser/components/search/content/search.xml;
/cvsroot/mozilla/browser/components/search/content/search.xml,v  <--  search.xml
new revision: 1.108; previous revision: 1.107
done

Comment 21

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120704 Minefield/3.0b2pre and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120704 Minefield/3.0b2pre.