Open Bug 1502975 Opened Last year Updated 5 months ago

Yandex search shortcut displayed in Russian for Turkish

Categories

(Firefox :: New Tab Page, enhancement, P1)

enhancement

Tracking

()

ASSIGNED
Iteration:
68.3 - Apr 15 - 28
Tracking Status
firefox65 --- wontfix
firefox66 --- fix-optional

People

(Reporter: selim, Assigned: mkaply, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

Attached image yandex-shortcut-tr.jpg
The pinned search shortcut for Yandex is displayed in Russian (Cyrillic script) for tr. Is should be @Yandex in Latin script.
Component: tr / Turkish → Activity Streams: Newtab
Product: Mozilla Localizations → Firefox
Summary: Yandex search shortcut displayed in Russian → Yandex search shortcut displayed in Russian for Turkish
Hmm.. there's two places in the screenshot that show russian, although the one in the address bar is the value from the page. The page's value comes from:

https://github.com/mozilla/activity-stream/blob/2076b6d27823afbdeef4abcfc0efe79a40acd13b/lib/SearchShortcuts.jsm#L14

If we get it to show @yandex, the address bar search should just work as it adds 2 internal aliases:

https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/toolkit/components/search/nsSearchService.js#968
How about seperating Yandex Turkey from Yandex Russia just like Amazon?
Iteration: --- → 65.2 (Nov 16)
Priority: -- → P2
Bug 1482125 added the internal aliases by doing a prefix match for "yandex" which includes "yandex-tr":
https://searchfox.org/mozilla-central/rev/eac6295c397133b7346822ad31867197e30d7e94/toolkit/components/search/nsSearchService.js#968-978

Potentially if we add an entry for "yandex-tr" to return "@yandex" and not also "@Яндекс", activity stream trying to add "@Яндекс" would fail because there's no engine with that alias but then could try to find an engine with "@yandex" instead.
Blocks: 1479806, 1496772
Depends on: 1482125
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Priority: P2 → P1
> Potentially if we add an entry for "yandex-tr" to return "@yandex" and not also "@Яндекс", activity stream trying to add "@Яндекс" would fail because there's no engine with that alias but then could try to find an engine with "@yandex" instead.

Why would activity stream try to add @Яндекс? In the turkish case, it should use @yandex because that is the only alias available.
Even if only @yandex is added for yandex-tr instead of the current behavior of additionally adding @Яндекс, Activity Stream's current configuration doesn't specify locales and instead it looks up available search engines by aliases:

https://github.com/mozilla/activity-stream/blob/2617ba7e1decedb15ecf5e9dbce229554d28e39b/lib/SearchShortcuts.jsm#L8-L14

I.e., the code tries to add @amazon, @百度, @google and @Яндекс for all users and happens to skip baidu and yandex for en-US because those search aliases aren't available.

I suppose the logic could change to match by.. url? and assume the first internal alias is the one that should be displayed in the content page?
Side note, how can I test this code? I keep getting Google as a search tile, not Yandex.
I'm not clear how this matching is working anyway:

  {keyword: "@amazon", shortURL: "amazon", url: "https://amazon.com"},
  {keyword: "@\u767E\u5EA6", shortURL: "baidu", url: "https://baidu.com"},
  {keyword: "@google", shortURL: "google", url: "https://google.com"},
  {keyword: "@\u044F\u043D\u0434\u0435\u043A\u0441", shortURL: "yandex", url: "https://yandex.com"},

If you only use google.com, you're not converting any non US google URLs. And it's not https://google.com, it's https://www.google.com

Amazon is also missing all the non US amazons.

And for Yandex, they use completely different URLs for different countries

https://yandex.ru/
https://yandex.com.tr/

etc.
The url is a dummy placeholder I believe… for blocking the tile.

Here's the actual logic that matches _internalAliases for the keyword in the configuration:

https://github.com/mozilla/activity-stream/blob/73f173931bd55c1e122454be563ed53b769041ff/lib/TopSitesFeed.jsm#L381-L386
It seems to be used to create the search tile even if the search engine being used doesn't correspond to that URL.

I opened:

https://bugzilla.mozilla.org/show_bug.cgi?id=1504764
(In reply to Mike Kaply [:mkaply] from comment #7)
> Side note, how can I test this code? I keep getting Google as a search tile,
> not Yandex.

You can test the address bar autocomplete behavior by getting the appropriate default engines updated with this in the Browser Console…

Services.locale.requestedLocales = Services.locale.availableLocales = ["tr"];

On nightly without your patch, I see an autocomplete suggestion for "@Яндекс - Search with Yandex" and with the patch I see "@yandex - Search with Yandex" (Note that the short name in both cases is the one from yandex-tr. Also, it looks like the autocomplete is cached, so let the engines load before typing "@" into the address bar.)


Then you can trigger activity stream to try to add yandex by changing from about:config:

browser.newtabpage.activity-stream.improvesearch.topSiteSearchShortcuts.searchEngines
set that to "yandex"

Without your patch, I see @Яндекс added and with your patch, yandex isn't added at all (because activity stream is currently only looking for "@Яндекс" alias.)
> Without your patch, I see @Яндекс added and with your patch, yandex isn't added at all (because activity stream is currently only looking for "@Яндекс" alias.)

Can you provide more context on this? I see no reference to Яндекс in the code.

Shouldn't it just be a matter of using locale-specific aliases here?

https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/ActivityStream.jsm#182
> Can you provide more context on this? I see no reference to Яндекс in the code.

In the activity stream code.
OK, I think I understand all of this. We really shouldn't be using URLs to identify these things.

The code that sets "searchEngines" isn't really setting searchEngines, it's setting top level domains and then using those to map to search shortcuts here

https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/SearchShortcuts.jsm#11

That's equating search engines with search URLs which is a false equivalence.

We should either use search engines or search URLs, but we really shouldn't mix and match.
Attachment #9022681 - Attachment is obsolete: true
The original approach in attachment 9022681 [details] fixes the problem in this screenshot.
Doh. We'll need both patches. I'll update.
Iteration: 65.2 (Nov 16) → 65.3 (Nov 30)
Iteration: 65.3 (Nov 30) → 66.2 - Dec 24 - Jan 6
Iteration: 66.2 - Dec 24 - Jan 6 → 66.3 - Jan 7 - 20
Iteration: 66.3 - Jan 7 - 20 → 67.1 - Jan 28 - Feb 10
Iteration: 67.1 - Jan 28 - Feb 10 → 67.3 - Feb 25 - Mar 10
Iteration: 67.3 - Feb 25 - Mar 10 → 67.4 - Mar 11 - 17
Iteration: 67.4 - Mar 11 - 17 → 68.1 - Mar 18 - 31
Iteration: 68.1 - Mar 18 - 31 → 68.2 - Apr 1 - 14
Attachment #9022681 - Attachment is obsolete: false

So for now, the only thing that should go in to fix this is the original patch that Mardak approved:

Bug 1502975 - Be explicit about yandex search aliases

This fixes the exact case in this bug (which is what we should be doing).

Can I check that in?

Just to be clear, with the first attachment, turkish users won't get any yandex search shortcut, and that's okay, yes? (But can still use @yandex from the address bar.)

I didn't think search shortcuts were turned on worldwide. Are they?

Default search shortcuts are on by region. I'm guessing that's how the reporter ran into this bug. Yandex is added for BY KZ RU TR:

https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/browser/components/newtab/lib/ActivityStream.jsm#190-191

I just tested a tr build forcing browser.search.region to "TR" and I see a default pinned @яндекс search shortcut.

Just to be clear, with the first attachment, turkish users won't get any yandex search shortcut, and that's okay, yes? (But can still use @yandex from the address bar.)

Yes, that's OK. because we can't fix that until we fix bug 1504764 because we don't have the right domain.

Iteration: 68.2 - Apr 1 - 14 → 68.3 - Apr 15 - 28
Component: Activity Streams: Newtab → New Tab Page

Any updates about landing the patch?

Even though this bug doesn't affect functionality, seeing foreign text in the start page wouldn't be a good first impression.

Flags: needinfo?(mozilla)
See Also: → 1562843
You need to log in before you can comment on or make changes to this bug.