Opened 10 years ago

Closed 9 years ago

#2218 closed New Feature (fixed)

Improve browser detection

Reported by: Wiktor Walc Owned by: Martin Kou
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 (4)

2218.patch (2.5 KB) - added by Martin Kou 10 years ago.
2218_2.patch (2.7 KB) - added by Martin Kou 9 years ago.
2218_3.patch (2.5 KB) - added by Martin Kou 9 years ago.
2218_4.patch (2.6 KB) - added by Martin Kou 9 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 10 years ago by Wojciech Olchawa

Keywords: Confirmed added
Version: SVN

comment:2 Changed 10 years ago by Thilo Schwarz

Component: GeneralServer : Java
Milestone: FCKeditor 2.6.1FCKeditor.Java 2.4
Owner: set to Thilo Schwarz

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

Component: Server : JavaGeneral
Milestone: FCKeditor.Java 2.4FCKeditor 2.6.1
Owner: Thilo Schwarz deleted

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 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Thilo 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 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Martin Kou

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 10 years ago by Martin Kou

Attachment: 2218.patch added

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:9 Changed 10 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:10 Changed 10 years ago by Frederico Caldeira Knabben

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 9 years ago by Martin Kou

Attachment: 2218_2.patch added

comment:11 Changed 9 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:12 Changed 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Martin Kou

Attachment: 2218_3.patch added

comment:13 Changed 9 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:14 Changed 9 years ago by Martin Kou

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 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Martin Kou

Attachment: 2218_4.patch added

comment:16 Changed 9 years ago by Martin Kou

Keywords: Review? added; Review- removed

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

comment:17 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:18 Changed 9 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [2053].

Click here for more info about our SVN system.

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