643172 - Some searchbar cleanup found by SeaMonkey reviews

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Robert Kaiser]

In bug 401417, the SeaMonkey reviews on search bar and OpenSearch engine manager found a few cleanups worthwhile to re-integrate into the Firefox copies as well IMHO. I have that work already on my disk. :)

Comment 1

[Robert Kaiser]

Attachment: v1: all cleanups from the SeaMonkey work

So, here's a patch with all those cleanups I did on the SeaMonkey side. This includes:
- various trailing-whitespace and small code style fixes
- Services.jsm-ify engine manager
- remove observer in engine manager onunload
- only invalidate where needed in observe
- fix bugs with duplicate engines and re-using a "let" variable outside its scope
- slightly simplify onSelect
- make _cloneEngine easier to understand (better variable name)
- no need to clear selection before setting it to one explicit row
- remove two unneeded (according to Neil) spacers in engine manager
- add plain class for searchbar-engine-button in search bar, obsoleting some previously explicit CSS rules
- more efficiently use field for searchService property (and use an easier understandable name for the field)
- group searchButton field with the others
- don't set textbox label where no label present anyhow
- use .classList.contains() instead of .getAttribute("class").indexOf()
- use nsIPrefBranch2 for _prefbranch field and use that for observers as well
- move init of field values into fields themselves as they lazily init anyhow
- remove unneeded !important

I hope that list help to make review easier :)

Comment 2

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

This would be much easier to review in small chunks.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 520471 [details] [diff] [review]
v1: all cleanups from the SeaMonkey work

>+      <field name="_searchService">
>+        Components.classes["@mozilla.org/browser/search-service;1"]
>+                  .getService(Components.interfaces.nsIBrowserSearchService);
>+      </field>

>+      <property name="searchService" readonly="true"
>+                onget="return this._searchService;"/>

Any reason why this is different from the following?

<field name="searchService" readonly="true">
  Components.classes["@mozilla.org/browser/search-service;1"]
            .getService(Components.interfaces.nsIBrowserSearchService);
</field>


>--- a/browser/themes/gnomestripe/browser/searchbar.css
>+++ b/browser/themes/gnomestripe/browser/searchbar.css

>-.autocomplete-textbox-container {
>-  -moz-box-align: stretch;
>-}

> .searchbar-engine-button > .button-box {
>-  -moz-appearance: none;
>   padding: 2px 0;
>   -moz-padding-end: 2px;
>-  border: 0;
> }

>--- a/browser/themes/winstripe/browser/searchbar.css
>+++ b/browser/themes/winstripe/browser/searchbar.css

>-.autocomplete-textbox-container {
>-  -moz-box-align: stretch;
>-}

>-.searchbar-engine-button > .button-box {
>-  -moz-appearance: none;
>-  padding: 0;
>-  border: 0;

I don't understand these changes.

Comment 4

[[e-mail]]

(In reply to comment #3)
> <field name="searchService" readonly="true">
>   Components.classes["@mozilla.org/browser/search-service;1"]
>             .getService(Components.interfaces.nsIBrowserSearchService);
> </field>
Not an answer to your question, but fields can't be readonly.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> (In reply to comment #3)
> > <field name="searchService" readonly="true">
> >   Components.classes["@mozilla.org/browser/search-service;1"]
> >             .getService(Components.interfaces.nsIBrowserSearchService);
> > </field>
> Not an answer to your question, but fields can't be readonly.

see bug 542406

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> see bug 542406

Nice, will take a look at that further cleanup!

(In reply to comment #3)
> >-.autocomplete-textbox-container {
> >-  -moz-box-align: stretch;
> >-}
> 
> >-.searchbar-engine-button > .button-box {
> >-  -moz-appearance: none;
> >-  padding: 0;
> >-  border: 0;
> 
> I don't understand these changes.

The -moz-box-align is not needed any more with the removal of the unneeded spacers, I think. And Neil said he doesn't see the point of the rule to be there in the first place.

The removal of the button rules is simply because of the addition of the "plain" class on the button! Firefox tends to have a lot of places where it sets such things manually all over when just using the "plain" class would be much nicer. :)

Comment 7

[Robert Kaiser]

(In reply to comment #2)
> This would be much easier to review in small chunks.

Hmm, I hope you still will review this, as with the current amount of time that I can free up for things like that, it would probably take me a few weeks to come up with the almost 20 separate patches to keep every point I mention in comment #1 as a separate chunk. It's not that easy to pick those things apart again.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (In reply to comment #3)
> > >-.autocomplete-textbox-container {
> > >-  -moz-box-align: stretch;
> > >-}
> > 
> > >-.searchbar-engine-button > .button-box {
> > >-  -moz-appearance: none;
> > >-  padding: 0;
> > >-  border: 0;
> > 
> > I don't understand these changes.
> 
> The -moz-box-align is not needed any more with the removal of the unneeded
> spacers, I think.

You're not removing spacers from the textbox.

> And Neil said he doesn't see the point of the rule to be
> there in the first place.

The autocomplete-textbox-container rule overrides -moz-box-align: center; from autocomplete.css.

> The removal of the button rules is simply because of the addition of the
> "plain" class on the button! Firefox tends to have a lot of places where it
> sets such things manually all over when just using the "plain" class would be
> much nicer. :)

You're removing stuff from .button-box, which doesn't have the class.

Comment 9

[[e-mail]]

(In reply to comment #3)
> >-.autocomplete-textbox-container {
> >-  -moz-box-align: stretch;
> >-}
> 
> >-.searchbar-engine-button > .button-box {
> >-  -moz-appearance: none;
> >-  padding: 0;
> >-  border: 0;
> 
> I don't understand these changes.

The button box went away in our version because I switched to a plain toolbarbutton which is plainer than a plain, um, standard plain button.

The autocomplete textbox container doesn't need to stretch because the textbox-input-box and the search-go-container are already centred. (So we could in fact then remove that style from the search-go-container as well.)

Comment 10

[Dão Gottwald [:dao]]

Stretching autocomplete-textbox-container lets the engine button fill the textbox (instead of filling it only when the font size is small enough).

Comment 11

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

(In reply to comment #7)
> Hmm, I hope you still will review this, as with the current amount of time that
> I can free up for things like that, it would probably take me a few weeks to
> come up with the almost 20 separate patches to keep every point I mention in
> comment #1 as a separate chunk.

You don't need to split it into 20 patches. 3 patches would still be better.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> You don't need to split it into 20 patches. 3 patches would still be better.

The only thing I think I can reasonably do after the fact is to separate the engine manager changes out from the searchbar bits and I'll do that in the next few minutes. I'll also undo the stuff dao has disputed, I'm not willing to go into long discussions over this, I just wanted to help.

Comment 13

[Robert Kaiser]

Attachment: searchbar part, v2

Here's only the searchbar part, corrected for Dao's comments.

Comment 14

[Robert Kaiser]

Attachment: engine manager part, v2

And here's the engine manager part.

I have tested all functionality I could access in those two UI items and all seems fine to me.

Comment 15

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

Comment on attachment 534227 [details] [diff] [review]
searchbar part, v2

Most of the changes look OK, but the button styling to use "plain" breaks the search bar appearance on Mac (and maybe Windows/Linux as well, haven't tested those).

Comment 16

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

(the problem is likely the removal of the button's margin)

Comment 17

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

Comment on attachment 534229 [details] [diff] [review]
engine manager part, v2

Some of these changes are obviously fine and easy to review, but a bunch of other ones are not. I really don't have the time to sort through this all, unfortunately, and given the lack of test coverage for much of this code I don't feel comfortable taking this without detailed review. If you split this up into simpler parts, I can review it then, but otherwise this is never going to be a priority, and so I'm going to take it out of my queue.

Comment 18

[Robert Kaiser]

(In reply to comment #15)
> Comment on attachment 534227 [details] [diff] [review] [review]
> searchbar part, v2
> 
> Most of the changes look OK, but the button styling to use "plain" breaks
> the search bar appearance on Mac (and maybe Windows/Linux as well, haven't
> tested those).

Could you be more specific than "breaks"?

(In reply to comment #16)
> (the problem is likely the removal of the button's margin)

Unlikely, as plain sets a 0 margin. But I'll look into it when I have time.

(In reply to comment #17)
> Comment on attachment 534229 [details] [diff] [review] [review]
> engine manager part, v2
> 
> Some of these changes are obviously fine and easy to review, but a bunch of
> other ones are not. I really don't have the time to sort through this all,
> unfortunately, and given the lack of test coverage for much of this code I
> don't feel comfortable taking this without detailed review. If you split
> this up into simpler parts, I can review it then, but otherwise this is
> never going to be a priority, and so I'm going to take it out of my queue.

That's the lamest excuse for an r- I have seen so far, but I'll look at it when I have time in the next year or two. Note that this attitude is why I care less and less to help the actual Firefox code - esp. as I'm paid nowadays for working on areas of Firefox where I feel I can actually make a difference and am not confronted by such blunt statements when I try to help.

Comment 19

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

The pinstripe theme uses non-zero padding/margins:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/searchbar.css#5
.plain overrides these with !important rules.

I'm sorry you're upset about this, but none of these fixes are critical, and they take me significant time to review, partially because a) our test coverage for the search bar isn't complete and b) you quite obviously don't test them before asking for review. The fact that many unrelated changes are bundled together in one patch exacerbates this. It really shouldn't be very difficult to split these changes up, but if you can't find the time to do it because you're busy with other more important things (comment 7), then I hope you can understand that I may also have other more important things to work on.

Comment 20

[Robert Kaiser]

(In reply to comment #19)
> b) you quite obviously don't
> test them before asking for review.

This isn't correct. I did test in fact, but I just can't test on Mac or Windows, and pinstripe obviously is therefore out of my reach for testing. :(

Comment 21

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

OK, sorry about that. I was misremembering bug 600244 comment 8.

Comment 22

[Robert Kaiser]

Comment on attachment 534229 [details] [diff] [review]
engine manager part, v2

I'm moving engine manager to bug 709589.

Comment 23

[Robert Kaiser]

Attachment: fields, v1

OK, I have split the search bar part alone into 4 parts.

Here's the first one, pulling together all the fields, moving their initialization into their constructors and using the _prefBranch field instead of initializing another separate object.

Comment 24

[Robert Kaiser]

Attachment: button, v1

Here's the apparently slightly more controversial one, putting the plain class onto the button and removing then-redundant CSS.

Comment 25

[Robert Kaiser]

Attachment: whitespace and nits, v1

Here's the part that is only cleaning up whitespace, some unused lines and some other tiny style nits.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 580749 [details] [diff] [review]
fields, v1

How about Services.prefs instead of this._prefBranch?

Comment 27

[Robert Kaiser]

Attachment: classList, v1

And here's the last part, using .classList instead of string manipulations on the "class" attribute.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 580751 [details] [diff] [review]
button, v1

Missed pinstripe?

Comment 29

[Robert Kaiser]

(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 580749 [details] [diff] [review]
> fields, v1
> 
> How about Services.prefs instead of this._prefBranch?

Not sure if it's available here at this moment, and actually, I'm so tired of seeing this bug lying around that the only reason why I didn't just WONTFIX it and throw it away is because I still care about Firefox. In any case, I'd prefer to have this in a followup if possible.

Comment 30

[Robert Kaiser]

(In reply to Dão Gottwald [:dao] from comment #28)
> Comment on attachment 580751 [details] [diff] [review]
> button, v1
> 
> Missed pinstripe?

Found any rules I can remove there or is this just a guess?

Comment 31

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

(In reply to Robert Kaiser (:[e-mail]) from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #28)
> > Comment on attachment 580751 [details] [diff] [review]
> > button, v1
> > 
> > Missed pinstripe?
> 
> Found any rules I can remove there or is this just a guess?

Dao's referring to http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/searchbar.css#5 . That button actually has a padding/margin on Mac, does this change not break that?

Comment 32

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

Comment on attachment 580751 [details] [diff] [review]
button, v1

(as mentioned in comment 15, I guess)

Comment 33

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

Comment on attachment 580749 [details] [diff] [review]
fields, v1

r=me, assuming you've built with this patch and manually tested the relevant functionality.

Comment 34

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

Comment on attachment 580753 [details] [diff] [review]
classList, v1

There seem to be a couple of setAttribute("class") that you could replace with classList.add too, right? again, r=me if you've tested this manually.

Comment 35

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

Comment on attachment 580752 [details] [diff] [review]
whitespace and nits, v1

>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

>       <method name="updateDisplay">

>           var name = this.currentEngine.name;
>           var text = this._stringBundle.getFormattedString("searchtip", [name]);
>           this._textbox.placeholder = name;
>-          this._textbox.label = text;

This isn't really a "whitespace change" :). This looks like a behavior change compared to the current code (the label getter falls back to placeholder, which is set here to "name", not "text").

Unless I'm missing something, please omit this change. r=me with that.

Comment 36

[Robert Kaiser]

A try push found opt a11y tests to not like those patches (debug are fine). I need to investigate that further.

Comment 37

[Robert Kaiser]

As https://tbpl.mozilla.org/?tree=Try&rev=3680ca4e3d4d shows, the a11y test breakages is coming from the fields patch.
If I can reasonably turn the order of the patches around, I'll do a try push for the other two and land them if green.

Comment 38

[Robert Kaiser]

The whitespace and classList patches looked good on try, so I pushed those:

http://hg.mozilla.org/integration/mozilla-inbound/rev/0d34a3506644
http://hg.mozilla.org/integration/mozilla-inbound/rev/0b5e42a04630

Will need to take a closer look on that fields one and how it breaks a11y tests.

Comment 39

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/e0317e9fe11d
https://hg.mozilla.org/mozilla-central/rev/0b5e42a04630

Comment 40

[Robert Kaiser]

So, from all I'm seeing, the fields patch is disrupting the tests starting at this line:
http://mxr.mozilla.org/comm-central/source/mozilla/accessible/tests/mochitest/events/test_focus_autocomplete.xul#430

Here's the output related to those tests:

3243 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | wrong state bits for [' no node info ', role: entry, address: [xpconnect wrapped nsIAccessible]]!
3244 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | Focussed [' no node info ', role: entry, address: [xpconnect wrapped nsIAccessible]] must be focusable!
3245 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | Editable [' no node info ', role: entry, address: [xpconnect wrapped nsIAccessible]] cannot be readonly!
3246 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | Singleline [' no node info ', role: entry, address: [xpconnect wrapped nsIAccessible]] cannot be multiline!
3247 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | test with ID = '[ ' no node info ' ] focus' failed. No focus event.
3248 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | test with ID = '[ ' no node info ' ] focus' failed. There is unexpected focus event.
3249 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | test with ID = '[ ' no node info ' ] 'down ' key' failed. There is unexpected focus event.
3250 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | test with ID = '[ ' no node info ' ] 'down ' key' failed. There is unexpected focus event.
3251 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_focus_autocomplete.xul | Test timed out.

Comment 41

[Robert Kaiser]

Alex Surkov, any idea why attachment 580749 [details] [diff] [review] would cause the failures I'm talking about in comment #40?

Comment 42

[Dão Gottwald [:dao]]

Not sure if that's the reason for the failure, but it looks like that test needs the same treatment that tabbrowser tests got in bug 719754.

Comment 43

[alexander :surkov (:asurkov)]

Is it intermittent failure? I don't see oranges on mc for 0b5e42a04630 changeset. Where can I see full log?

Comment 44

[alexander :surkov (:asurkov)]

(In reply to Dão Gottwald [:dao] from comment #42)
> Not sure if that's the reason for the failure, but it looks like that test
> needs the same treatment that tabbrowser tests got in bug 719754.

It needs but I'm not sure it helps in this case. It sounds like one of synthDownKey fails (can't be certain which one without full log). We don't get a11y focus event for expected menuitem of popup. It sounds like the problem that popup is rebuild dynamically (rebuildPopupDynamic which was tweaked by 0b5e42a04630 changeset).

Comment 45

[Robert Kaiser]

(In reply to alexander :surkov from comment #43)
> Is it intermittent failure? I don't see oranges on mc for 0b5e42a04630
> changeset. Where can I see full log?

https://tbpl.mozilla.org/?tree=Try&rev=3680ca4e3d4d - I never checked it into m-c because it caused failures on Linux and Windows consistently every timed I pushed something with it to try (the link is the one try push where I used only this one patch to be sure).

Comment 46

[alexander :surkov (:asurkov)]

got it, I'll take a look

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 580749 [details] [diff] [review]
fields, v1

nsIPrefBranch2 is obsolete.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #46)
> got it, I'll take a look

fields v1 patch doesn't applied cleanly against trunk

Comment 49

[Robert Kaiser]

(In reply to Dão Gottwald [:dao] from comment #47)
> Comment on attachment 580749 [details] [diff] [review]
> fields, v1
> 
> nsIPrefBranch2 is obsolete.

Thanks for the reminder, I fixed that locally and that's why it doesn't apply cleanly, but the tests failed before this was cared about tree-wide. I'll attach a patch in a minute that has the fix for this, so surkov can apply it cleanly.

Comment 50

[Robert Kaiser]

Attachment: fields, v1.1

This is a version that applies cleanly on trunk (removing the "2" from nsIPrefBranch), forwarding the r+ as it's still the same as v1 for everything else (but I expect something else needed to be done either in the patch or in the test to fix the a11y orange seen on try).

Comment 51

[alexander :surkov (:asurkov)]

(In reply to Robert Kaiser (:[e-mail]) from comment #50)
> Created attachment 600867 [details] [diff] [review]
> fields, v1.1

it seems it's running fine locally, let me do a try server build.

Comment 52

[alexander :surkov (:asurkov)]

Attachment: improve a11y logging

Comment 53

[alexander :surkov (:asurkov)]

HTML form autocomplete tests fails (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/events/test_focus_autocomplete.xul#424) at up key press test (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/events/test_focus_autocomplete.xul#327).

HTML form autocomplte doesn't get initialized (no popup), see initFormAutoCompleteBy. No idea why this can fail.

Comment 54

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #52)
> Created attachment 600916 [details] [diff] [review]
> improve a11y logging

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab013bc54d26

Comment 55

[Ed Morley [:emorley]]

(In reply to alexander :surkov from comment #54)
> (In reply to alexander :surkov from comment #52)
> > Created attachment 600916 [details] [diff] [review]
> > improve a11y logging
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ab013bc54d26

https://hg.mozilla.org/mozilla-central/rev/ab013bc54d26

Comment 56

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

Kairo: is this bug tracking anything useful anymore?

Comment 57

[Robert Kaiser]

This is a work bug, not a tracking one, and unfortunately, I didn't hear of any results of the analysis of the a11y orange from :surkov, so the fields patch still remains unlanded. At some point, it would be good to get that in as well.

Comment 58

[Robert Kaiser]

The old search code has been removed, AFAIK, so let's call this fixed for what landed and forget what bounced back then.