Some searchbar cleanup found by SeaMonkey reviews

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Search
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

Trunk
Firefox 13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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. :)
(Assignee)

Comment 1

7 years ago
Created attachment 520471 [details] [diff] [review]
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 :)
Attachment #520471 - Flags: review?(gavin.sharp)
This would be much easier to review in small chunks.
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

7 years ago
(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.
(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
(Assignee)

Comment 6

7 years ago
(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. :)
(Assignee)

Comment 7

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

7 years ago
(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.)
Stretching autocomplete-textbox-container lets the engine button fill the textbox (instead of filling it only when the font size is small enough).

Updated

7 years ago
Attachment #520471 - Flags: review?(gavin.sharp) → review-
(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.
(Assignee)

Comment 12

7 years ago
(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.
(Assignee)

Comment 13

7 years ago
Created attachment 534227 [details] [diff] [review]
searchbar part, v2

Here's only the searchbar part, corrected for Dao's comments.
Attachment #520471 - Attachment is obsolete: true
Attachment #534227 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

7 years ago
Created attachment 534229 [details] [diff] [review]
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.
Attachment #534229 - Flags: review?(gavin.sharp)
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).
Attachment #534227 - Flags: review?(gavin.sharp) → review-
(the problem is likely the removal of the button's margin)
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.
Attachment #534229 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 18

7 years ago
(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.
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.
(Assignee)

Comment 20

7 years ago
(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. :(
OK, sorry about that. I was misremembering bug 600244 comment 8.
(Assignee)

Comment 22

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

I'm moving engine manager to bug 709589.
Attachment #534229 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Created attachment 580749 [details] [diff] [review]
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.
Attachment #534227 - Attachment is obsolete: true
Attachment #580749 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

6 years ago
Created attachment 580751 [details] [diff] [review]
button, v1

Here's the apparently slightly more controversial one, putting the plain class onto the button and removing then-redundant CSS.
Attachment #580751 - Flags: review?(gavin.sharp)
(Assignee)

Comment 25

6 years ago
Created attachment 580752 [details] [diff] [review]
whitespace and nits, v1

Here's the part that is only cleaning up whitespace, some unused lines and some other tiny style nits.
Attachment #580752 - Flags: review?(gavin.sharp)
Comment on attachment 580749 [details] [diff] [review]
fields, v1

How about Services.prefs instead of this._prefBranch?
(Assignee)

Comment 27

6 years ago
Created attachment 580753 [details] [diff] [review]
classList, v1

And here's the last part, using .classList instead of string manipulations on the "class" attribute.
Attachment #580753 - Flags: review?(gavin.sharp)
Comment on attachment 580751 [details] [diff] [review]
button, v1

Missed pinstripe?
(Assignee)

Comment 29

6 years ago
(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.
(Assignee)

Comment 30

6 years ago
(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?
(Assignee)

Updated

6 years ago
Summary: Some searchbar and engine manager cleanup found by SeaMonkey reviews → Some searchbar cleanup found by SeaMonkey reviews
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 on attachment 580751 [details] [diff] [review]
button, v1

(as mentioned in comment 15, I guess)
Attachment #580751 - Flags: review?(gavin.sharp) → review-
Comment on attachment 580749 [details] [diff] [review]
fields, v1

r=me, assuming you've built with this patch and manually tested the relevant functionality.
Attachment #580749 - Flags: review?(gavin.sharp) → review+
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.
Attachment #580753 - Flags: review?(gavin.sharp) → review+
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.
Attachment #580752 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 36

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

Comment 37

6 years ago
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.
(Assignee)

Comment 38

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/e0317e9fe11d
https://hg.mozilla.org/mozilla-central/rev/0b5e42a04630
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 40

6 years ago
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.
(Assignee)

Comment 41

6 years ago
Alex Surkov, any idea why attachment 580749 [details] [diff] [review] would cause the failures I'm talking about in comment #40?
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

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

Comment 44

6 years ago
(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).
(Assignee)

Comment 45

6 years ago
(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

6 years ago
got it, I'll take a look
Comment on attachment 580749 [details] [diff] [review]
fields, v1

nsIPrefBranch2 is obsolete.

Comment 48

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

fields v1 patch doesn't applied cleanly against trunk
(Assignee)

Comment 49

6 years ago
(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.
(Assignee)

Comment 50

6 years ago
Created attachment 600867 [details] [diff] [review]
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).
Attachment #580749 - Attachment is obsolete: true
Attachment #600867 - Flags: review+

Comment 51

6 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) 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

6 years ago
Created attachment 600916 [details] [diff] [review]
improve a11y logging
Attachment #600916 - Flags: review?(marco.zehe)

Comment 53

6 years ago
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.

Updated

6 years ago
Attachment #600916 - Flags: review?(marco.zehe) → review+

Comment 54

6 years ago
(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
Whiteboard: [don't mark fixed]
(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
Kairo: is this bug tracking anything useful anymore?
(Assignee)

Comment 57

5 years ago
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.
(Assignee)

Comment 58

2 years ago
The old search code has been removed, AFAIK, so let's call this fixed for what landed and forget what bounced back then.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Whiteboard: [don't mark fixed]
You need to log in before you can comment on or make changes to this bug.