1063193 - Performing searches with Wikipedia search engine immediately launches Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[:Margaret Leibovic]

This is similar to the problem I was having for facets in bug 1062389.

We have logic in place that launches Fennec if the host of the URL we're trying to load doesn't match the host of the original search URL, but sometimes there can be redirects to other sub-domains.

I feel like we need a better way to determine whether or not to open Fennec, but I'm not sure what that should be.

Comment 1

[Aaron Train [:aaronmt]]

I get this in different locales too such as ru_RU and fr_FR I just tested. All searches just launch the browser.

Comment 2

[:Margaret Leibovic]

So, the search plugin specifies "https://en.wikipedia.org/wiki/Special:Search" as the template, but it redirects to "https://en.m.wikipedia.org/wiki/Special:Search".

Fixing the template would fix this issue (and it would avoid a redirect). mconnor, can we do that? This change would affect Fennec as well, but I think it would be a positive change.

However, interesting side effect of the way wikipedia searches work is that you'll probably never leave the search activity, since the results aren't a list of external websites. In fact, usually the results just redirect to an article. If you really love wikipedia, this seems like a good way to get fast results, but I'm not sure how it fits into Fennec's story. Maybe it will fit in better if we ever allow you to simultaneously search from multiple engines (or let you switch engines more easily).

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I think Wikipedia can UA sniff to switch between Phone and Tablet. If we switch the URL, let's make sure to add a tablet fallback, like we do in other XML files.

Comment 4

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> I think Wikipedia can UA sniff to switch between Phone and Tablet. If we
> switch the URL, let's make sure to add a tablet fallback, like we do in
> other XML files.

Testing on my N7, Wikipedia searches are still redirecting to en.m.wikipedia.com. I can add in a tablet <Url> tag like we do for Bing in case this changes, but it doesn't look like that would make a difference now.

Comment 5

[:Margaret Leibovic]

Attachment: Update Wikipedia search plugin so that it won't redirect to a new domain

Can I just do this? As I mentioned in my last comment, wikipedia doesn't serve a desktop site to tablets (also tested on a 10 inch tablet), so this will save us a redirect in Fennec as well.

Comment 6

[Mike Connor [:mconnor]]

If they're redirecting everything, this is the right fix, but we should probably tell them that's... lame.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8496244 [details] [diff] [review]
Update Wikipedia search plugin so that it won't redirect to a new domain

Right. Let's save a redirect for now. We can reach out to Wikipedia for a fix on their end so we get a better UX on 10" tablets.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/2b67ef67b6a9

flod, will localizers see this change? This is something we would probably want to update in localized search plugins as well.

Comment 9

[Francesco Lodolo [:flod]]

They'll see it, but they're also not authorized to make changes to searchplugins without review (unless we whitelist the change) :-\

And the worst part is that I finished fixing those URLs for bug 825821 not so long ago.

Comment 10

[:Margaret Leibovic]

(In reply to Francesco Lodolo [:flod] from comment #9)
> They'll see it, but they're also not authorized to make changes to
> searchplugins without review (unless we whitelist the change) :-\
> 
> And the worst part is that I finished fixing those URLs for bug 825821 not
> so long ago.

Oh, I didn't know about that bug. But mfinkle should have!

https://bugzilla.mozilla.org/show_bug.cgi?id=825821#c4

This makes me question the patch we went with in this bug. Maybe we should add the tablet specific URL, and allow that do to the redirect.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #10)

> > And the worst part is that I finished fixing those URLs for bug 825821 not
> > so long ago.
> 
> Oh, I didn't know about that bug. But mfinkle should have!
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=825821#c4
> 
> This makes me question the patch we went with in this bug. Maybe we should
> add the tablet specific URL, and allow that do to the redirect.

Zoinks! I'm shaking my fist at "Request Desktop Site". I'd be happy to r+ the phone/tablet split patch. Just make sure we fallback in a way that makes all 3 cases (phone/tablet/request-desktop-site) work.

Comment 12

[Mike Connor [:mconnor]]

If I go to the en.m.wikipedia.org URL with a desktop UA, I get redirected to the desktop site.  Same with Request Desktop Site.  I think the current patch is fine.

On the l10n issue, I wonder how hard it'd be to get Wikipedia to set up cnames so "{moz:locale}.m.wikipedia.org" Just Works?

Also, Wikipedia has greenlit using SSL, so we should be changing all of the links anyway...

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Mike Connor [:mconnor] from comment #12)
> Also, Wikipedia has greenlit using SSL, so we should be changing all of the
> links anyway...

That started a couple of weeks ago (bug 1069123).

To be honest I'm not sure how to reproduce this bug based on comment 0, but someone should check with the next en-US build if it actually breaks bug 825821 again.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b67ef67b6a9

Comment 15

[:Margaret Leibovic]

(In reply to Francesco Lodolo [:flod] from comment #13)
> (In reply to Mike Connor [:mconnor] from comment #12)
> > Also, Wikipedia has greenlit using SSL, so we should be changing all of the
> > links anyway...
> 
> That started a couple of weeks ago (bug 1069123).
> 
> To be honest I'm not sure how to reproduce this bug based on comment 0, but
> someone should check with the next en-US build if it actually breaks bug
> 825821 again.

Sigh, the change here does break bug 825821 again. I think I should just back this out.

However, that would mean we need to something else to solve our search activity issue. Maybe we should add a new <Url> form just to be used in the search activity? Or I can hard-code a fix in the search activity to ignore wikipedia re-directs, but that doesn't feel great. However, the nice part of that would be that we wouldn't need to update search plugins in all the locales.

mfinkle, what do you think?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #15)

> Sigh, the change here does break bug 825821 again. I think I should just
> back this out.
> 
> However, that would mean we need to something else to solve our search
> activity issue. Maybe we should add a new <Url> form just to be used in the
> search activity? Or I can hard-code a fix in the search activity to ignore
> wikipedia re-directs, but that doesn't feel great. However, the nice part of
> that would be that we wouldn't need to update search plugins in all the
> locales.

So many great choices from which to pick! I wouldn't mind experimenting with a new <Url> just to see if it would work. Maybe look at the "rel" type info that OpenSearch supports?

We can always do the easier/hackier hard coded fix.

Comment 17

[Flaviu Cos]

Verified as fixed in build 35.0a1 (2014-10-02);
Device Nexus 7 (Android 4.4.4);

Comment 18

[:Margaret Leibovic]

Backed out the first approach:
https://hg.mozilla.org/integration/fx-team/rev/39010225dc9f

I'll see what I can do with a new <Url>. Right now the parser just sets any type="text/html" as the results URI:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java#126

I'll look at the OpenSearch spec, but we could also do something hacky like type="application/x-moz-searchacvitiy".

Comment 19

[Mike Connor [:mconnor]]

Let's do it as a rel value please.  I'd like to kill the type hack (yeah, I need to file that bug) when the world is less insane.

Comment 20

[:Margaret Leibovic]

Attachment: Add mobile-specific URL to wikipedia search plugin

This fixes the issue by adding a new <Url rel="mobile"> to the Wikipedia plugin.

I wasn't really sure what to use as the rel value for this, but I chose "mobile" since that's what the URL actually reflects.

We currently don't really handle multiple URLs in the search plugin parser, so I just added some basic logic to handle parsing more than one URL, and to use this "mobile" URL if it exists.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8501424 [details] [diff] [review]
Add mobile-specific URL to wikipedia search plugin

I think this is a good progression to move more of the engines over to "rel" URLs. I suppose we could also create a <Url rel="tablet"> someday, when we need it.

MConnor might be pleased

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 8501424 [details] [diff] [review]
Add mobile-specific URL to wikipedia search plugin

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

Blegh, I can't say I like this since we're just building hacks on top of hacks. The fundamental problem here is relying on the domain to detect when we leave the search results page in the first place, which is fragile. For instance, search "analytics" with the Google search provider, and you don't leave the search activity when clicking the first result. But until we have providers giving us an API to build this thing the right way, we're kind of stuck.

Another option would be to have a custom set of "redirect rules", which doesn't sound too terrible to me since we already have the site-specific CSS rules. That would have the benefit of not needing to modify our search engine files specifically for the search activity, and would give us the ability to weed out edge cases like the example above.

Comment 23

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 8501424 [details] [diff] [review]
> Add mobile-specific URL to wikipedia search plugin
> 
> Review of attachment 8501424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Blegh, I can't say I like this since we're just building hacks on top of
> hacks. The fundamental problem here is relying on the domain to detect when
> we leave the search results page in the first place, which is fragile. For
> instance, search "analytics" with the Google search provider, and you don't
> leave the search activity when clicking the first result. But until we have
> providers giving us an API to build this thing the right way, we're kind of
> stuck.
> 
> Another option would be to have a custom set of "redirect rules", which
> doesn't sound too terrible to me since we already have the site-specific CSS
> rules. That would have the benefit of not needing to modify our search
> engine files specifically for the search activity, and would give us the
> ability to weed out edge cases like the example above.

Yeah... our current system is very much less than ideal :/

One thing that would be nice about the redirect rule is that we wouldn't need to update the localized plugins. But any real robust fix would likely involve updating the plugins anyway.

Now I'm having mixed feelings about this approach... maybe we should just try adding some additional logic in the isSearchResultsPage method.

Comment 24

[:Margaret Leibovic]

I decided to just land what we have here, and I filed bug 1081136 about trying to create a more robust solution in the future. However, that bug may require working with search providers to actually know whether or not a page is a "results" page.

https://hg.mozilla.org/integration/fx-team/rev/320dd3495026

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/320dd3495026

Comment 26

[Cristina Madaras, QA [:CristinaM]]

Performing searches with Wikipedia search engine doesn't launch Fennec. Verified as fixed in latest builds:
Firefox for Android 37.0a1 (2014-12-09)
Firefox for Android 36.0a2 (2014-12-09)
Firefox for Android 35.b1

Device: Nexus 4 (Android 4.4.4)