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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Search
P4
normal
RESOLVED FIXED
2 years ago
24 days ago

People

(Reporter: arni2033, Assigned: CactusTribe, Mentored)

Tracking

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

Firefox Tracking Flags

(firefox44 affected, firefox55 fixed)

Details

(Whiteboard: [fxsearch][good first bug], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8673210 [details]
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.
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>".
Mentor: florian@queze.net
Priority: -- → P4
Whiteboard: [dupeme] → [fxsearch]
(Reporter)

Comment 2

a year ago
Hm, probably bug 1113747 will make engine names always visible
Depends on: 1113747
(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.
(Reporter)

Updated

a year ago
Has STR: --- → yes
(Reporter)

Comment 4

a year ago
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.
I saw the search UI has been changed, is it still a valid issue?
Flags: needinfo?(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.
Flags: needinfo?(florian)
Whiteboard: [fxsearch] → [fxsearch][good first bug]

Comment 7

3 months ago
Hi, I'm interested in working on this bug if it's still available!
(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

3 months ago
(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!
(Assignee)

Comment 10

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

Comment 12

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

Comment 14

2 months ago
Created attachment 8845888 [details] [diff] [review]
fix.search.addEngine.tooltip.patch

First patch to fix issue 1214284
Attachment #8845888 - Flags: review?(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.
Attachment #8845888 - Flags: review?(florian) → feedback+
(Assignee)

Comment 16

2 months ago
Created attachment 8845917 [details] [diff] [review]
fix.search.addEngine.tooltip.patch

Second patch according to the feedback.
Attachment #8845888 - Attachment is obsolete: true
Attachment #8845917 - Flags: review?(florian)
Comment on attachment 8845917 [details] [diff] [review]
fix.search.addEngine.tooltip.patch

Thanks!
Attachment #8845917 - Flags: review?(florian) → review+
(Assignee)

Comment 18

a month ago
What is the next step for the patch to be merged? I have to do something more? 
Thank you for your time.
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.
(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.
Assignee: nobody → lefrancjoaquim

Comment 21

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fb03dd04a1d
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1352161
You need to log in before you can comment on or make changes to this bug.