Ticket #2218 (closed New Feature: fixed)

Opened 7 years ago

Last modified 6 years ago

Improve browser detection

Reported by: wwalc Owned by: martinkou
Priority: Normal Milestone: FCKeditor 2.6.1
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Review+ Cc:

Description

It seems that Epiphany browser is getting more and more popular recently. This browser is based on Gecko engine and is compatible with FCKeditor. However, as reported by yell0w at #fckeditor IRC channel: http://irc.fckeditor.net/index.php?date=2008-05-23 current implementation of browser detection doesn't detect it properly.

Epiphany introduces itself as

Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9b5) Gecko Epiphany/2.22

so searching for "Gecko/" fails.

Similar issue for FCKeditor.Java have been reported in #1744. There is another example of "user agent" that fails current browser detection:

Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.8.0.2) Gecko/Debian-1.5.dfsg+1.5.0.2-3 Firefox/1.5.0.2

Attachments

2218.patch (2.5 KB) - added by martinkou 6 years ago.
2218_2.patch (2.7 KB) - added by martinkou 6 years ago.
2218_3.patch (2.5 KB) - added by martinkou 6 years ago.
2218_4.patch (2.6 KB) - added by martinkou 6 years ago.

Change History

comment:1 Changed 7 years ago by w.olchawa

  • Keywords Confirmed added
  • Version set to SVN

comment:2 Changed 7 years ago by th-schwarz

  • Owner set to th-schwarz
  • Component changed from General to Server : Java
  • Milestone changed from FCKeditor 2.6.1 to FCKeditor.Java 2.4

comment:3 Changed 7 years ago by fredck

  • Owner th-schwarz deleted
  • Component changed from Server : Java to General
  • Milestone changed from FCKeditor.Java 2.4 to FCKeditor 2.6.1

Sorry Thilo, this is not a issue restricted to FCKeditor.Java. It impacts on all server side integration implementations, as we are all using a similar solution to detect Gecko. If you need a ticket to FCKeditor.Java only for your organization, feel free to create a new one.

comment:4 Changed 7 years ago by fredck

We need to verify if it also impacts the JavaScript implementation, which currently uses navigator.productSub to check it, instead of user agent string manipulation.

comment:5 Changed 7 years ago by th-schwarz

Oops sorry Fred , don't no why I switch that to FCKeditor.Java. Really Sorry!

To improve the compatibility check, we need a complete list of gecko based browsers, which are compatible to fckeditor. Not tested yet, but I think Epiphany isn't the only 'small' gecko based browser fckeditor could run with.

How about a collection of exemplary user agent strings, which have to fail or pass the check? That could be a nice base for a fine granular browser check in all sever side integration implementations.

comment:6 Changed 7 years ago by fredck

Let me clarify one thing so we are all at the same line.

There are a set of browsers to which we officially declare compatibility, and to those browsers we must guarantee it.

Then, there are other browsers that we are ok to support, but are out of our focus. Any effort to be compatible with them is optional.

To better clarify it, let's take a look at the V3 documentation regarding Browser Compatibility: http://docs.fckeditor.net/FCKeditor_3.x/Design_and_Architecture/Browsers_Compatibility

Note that for version 2.x, we are compatible with IE 5.5+ and FF 1.5+. Epiphany instead is part of the "Engine Compatible Browsers".

So, we don't have to be compatible with all Gecko based browsers right now. We can take a look at Epiphany, fixing the browser detection and testing the editor features to be sure it is fully compatible. If everything sounds good, then we can go ahead with it. Then the same effort can be extended to other browsers, "under request" though.

comment:7 Changed 7 years ago by martinkou

According to Mozilla's user agent string document, it seems they're thinking the other direction - the rv: x.y part is optional, but the Gecko/xxxxyyzz part is mandatory.

So, if some other Gecko based browser actually followed Mozilla's guideline, and we switched to detecting the rv: string, then our browser detection logic will break again. I guess a more careful approach is needed here - say, we can look for the rv: string first, and if that's not found, look for the Gecko/xxxxyyzz version.

Changed 6 years ago by martinkou

comment:8 Changed 6 years ago by martinkou

  • Keywords Review? added

comment:9 Changed 6 years ago by martinkou

  • Status changed from new to assigned
  • Owner set to martinkou

comment:10 Changed 6 years ago by fredck

  • Keywords Review- added; Review? removed

Including Mozilla 5.0 in the check seems to be a very bad thing. What happens when we'll have Mozilla 6.0?

I've found the "Browser Detection and Cross Browser Support" doc in the MDC. It specifies that the best way to identify Gecko is:

navigator.product == 'Gecko'

We already do that on fckeditor.js, so it would make sense here too. It also seems that all Gecko based browser return "Gecko", including Epiphany (should be checked, but see table), but also Safari and Opera (!), so we must exclude them.

Changed 6 years ago by martinkou

comment:11 Changed 6 years ago by martinkou

  • Keywords Review? added; Review- removed

comment:12 Changed 6 years ago by fredck

  • Keywords Review- added; Review? removed

It seems that the code can be much simpler.

  • Line 31 is not anymore needed at this point, as the IsGecko property is filled at line 41.
  • It seems the /rv:(\d+\.\d+)/ pattern is a constant for all Gecko based browsers. So, we don't need all that version stuff, converting the captured match to a float, doing simple numeric comparisons. (some comment cleanup is needed at this point).

Changed 6 years ago by martinkou

comment:13 Changed 6 years ago by martinkou

  • Keywords Review? added; Review- removed

comment:14 follow-up: ↓ 15 Changed 6 years ago by martinkou

The IsGecko assignment in line 41 actually depends on line 31, it's there to exclude those browsers who'd return navigator.product = 'Gecko'.

I've submitted a new patch to check for FF1.5 and FF3+ with only the rv:x.y string.

comment:15 in reply to: ↑ 14 Changed 6 years ago by fredck

  • Keywords Review- added; Review? removed

Replying to martinkou:

The IsGecko assignment in line 41 actually depends on line 31, it's there to exclude those browsers who'd return navigator.product = 'Gecko'.

Definitely, in the way you wrote it. But there is no need to make this check in two steps. What I meant is that line 41 could be changed to something like this:

browserInfo.IsGecko = ( navigator.product == 'Gecko' ) && !browserInfo.IsSafari && !browserInfo.IsOpera ;

Changed 6 years ago by martinkou

comment:16 Changed 6 years ago by martinkou

  • Keywords Review? added; Review- removed

Ok, I've fixed that in the new patch as well.

comment:17 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:18 Changed 6 years ago by martinkou

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [2053].

Click here for more info about our SVN system.

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy