1214284 - Can't tell the name of search engine I'm installing (not even by tooltip)

Status: RESOLVED FIXED

(Firefox :: Search, defect, P4)

Description

[arni2033]

Attachment: screenshot - Can't tell the name of search engine I'm installing (not even by tooltip).png

STR: [watch screenshot]
0. Install russian localization of Firefox (for better bug experience)
1. Set zoom level on your OS -> 125% (or set layout.css.devPixelsPerPx -> 1.25)
2. Restore defaults in Resize your window so that its width was <= 1366px
3. Go to https://slovari.yandex.ru/
4. Click the magnifier icon in searchbar
5. Add search engine "Яндекс.Словари. Русский язык"

Result:       Currently you can't do that because the name of search engine isn't visible
Expectations: User should see the name of search engine he's installing, _at_least_ in tooltip

Note:   I don't know how "tooltip" solution is supposed to work on touch devices.

Comment 1

[Florian Quèze [:florian]]

Currently the tooltip of these menuitems shows the URL of the file that will be downloaded if you click the item. I think it would make sense to change these tooltips to be "<engine name> - <url>".

Comment 2

[arni2033]

Hm, probably bug 1113747 will make engine names always visible

Comment 3

[Florian Quèze [:florian]]

(In reply to arni2033 from comment #2)
> Hm, probably bug 1113747 will make engine names always visible

I don't think that will help for the specific case in comment 0.

Comment 4

[arni2033]

Consider using "\n" as separator:   items on New Tab, history items/bookmarks in Library,
items in History panel use "\n" as separator for title and url.

Comment 5

[[e-mail]]

I saw the search UI has been changed, is it still a valid issue?

Comment 6

[Florian Quèze [:florian]]

(In reply to Fred Lin [:gasolin] from comment #5)
> I saw the search UI has been changed, is it still a valid issue?

Yes.

Comment 7

[Courtney]

Hi, I'm interested in working on this bug if it's still available!

Comment 8

[Florian Quèze [:florian]]

(In reply to Courtney from comment #7)
> Hi, I'm interested in working on this bug if it's still available!

Yes, it's still available. Do you already have a local copy of the source code?

The suggestion in comment 4 makes sense, we should display in these tooltips the engine name on the first line, and the url on the second line.

The code you want to modify is at http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/browser/components/search/content/search.xml#1667

Comment 9

[Courtney]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to Courtney from comment #7)
> > Hi, I'm interested in working on this bug if it's still available!
> 
> Yes, it's still available. Do you already have a local copy of the source
> code?
> 
> The suggestion in comment 4 makes sense, we should display in these tooltips
> the engine name on the first line, and the url on the second line.
> 
> The code you want to modify is at
> http://searchfox.org/mozilla-central/rev/
> 8fa84ca6444e2e01fb405e078f6d2c8da0e55723/browser/components/search/content/
> search.xml#1667

Yes, I have a local copy already. Thanks for your help!

Comment 10

[CactusTribe]

Hello, i'm new and for my first contribution i'm interested to fix this bug according with the comment 4.
This issue is currently unassigned, true? 
I have a local copy of code.

Comment 11

[Florian Quèze [:florian]]

(In reply to CactusTribe from comment #10)
> Hello, i'm new and for my first contribution i'm interested to fix this bug
> according with the comment 4.
> This issue is currently unassigned, true? 

Yes, you are welcome to write a patch and attach it here.

Comment 12

[CactusTribe]

Thanks ! Could you describe to me the procedure for a patch?
I follow this guide https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial ,but after the ./mach mercurial-setup i'm not sure what to do precisely.

hg commit "blabla"
hg push review
hg bzexport
hg export . > my-change.patch
and upload .patch file here ?

This is the procedure ?

Thanks

Comment 13

[Florian Quèze [:florian]]

(In reply to CactusTribe from comment #12)

> hg push review

> hg bzexport

> hg export . > my-change.patch
> and upload .patch file here ?

These are 3 different solutions that will lead to the same result: a patch showing up here. So pick one of the three and go ahead :-).

Comment 14

[CactusTribe]

Attachment: fix.search.addEngine.tooltip.patch

First patch to fix issue 1214284

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8845888 [details] [diff] [review]
fix.search.addEngine.tooltip.patch

Review of attachment 8845888 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch; it works well, but please address the following comment.

::: browser/components/search/content/search.xml
@@ +1698,5 @@
>                        this._fixUpEngineNameForID(engine.title);
>            let label = this.bundle.formatStringFromName("cmd_addFoundEngine",
>                                                         [engine.title], 1);
> +
> +          let label_tooltiptext = engine.title + "\n" + engine.uri;

To be consistent with the coding style in the rest of this code, the variable would be named 'labelTooltiptext' here, or maybe just 'tooltip'. But I don't think we really need this variable, as you can inline it without causing the button.setAttribute("tooltiptext", ... line to be longer than 80 characters.

Comment 16

[CactusTribe]

Attachment: fix.search.addEngine.tooltip.patch

Second patch according to the feedback.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8845917 [details] [diff] [review]
fix.search.addEngine.tooltip.patch

Thanks!

Comment 18

[CactusTribe]

What is the next step for the patch to be merged? I have to do something more? 
Thank you for your time.

Comment 19

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb03dd04a1d24db17662c9fc49e686aca55ee63
Bug 1214284 - Adding engine title and line separator in tooltip of button add search engine, r=florian.

Comment 20

[Florian Quèze [:florian]]

(In reply to CactusTribe from comment #18)
> What is the next step for the patch to be merged? I have to do something
> more?

The next step was me checking in the patch, which I just did (comment 19). I wanted to do this yesterday but got distracted, sorry about the delay.

Note: next time you update a patch, please make the new patch you attach not depend on the previous one (here attachment attachment 8845888 [details] [diff] [review] had to be applied before applying attachment 8845917 [details] [diff] [review]); thanks.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fb03dd04a1d