Last Comment Bug 799502 - Override the UA string for moodle to get the rich text editor rather than plain textareas
: Override the UA string for moodle to get the rich text editor rather than pla...
Status: REOPENED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://input.mozilla.org/de/?q=moodle...
Depends on: 797703
Blocks: 588909 815446
  Show dependency treegraph
 
Reported: 2012-10-09 07:56 PDT by Dão Gottwald [:dao]
Modified: 2012-12-04 05:32 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
patch (5.07 KB, patch)
2012-10-09 07:56 PDT, Dão Gottwald [:dao]
gerv: review+
bzbarsky: review+
felipc: review+
Details | Diff | Review
patch v2 (5.69 KB, patch)
2012-10-10 05:52 PDT, Dão Gottwald [:dao]
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Dão Gottwald [:dao] 2012-10-09 07:56:49 PDT
Created attachment 669535 [details] [diff] [review]
patch

+++ This bug was initially created as a clone of Bug #797703 +++

see bug 797703 for details
Comment 1 Boris Zbarsky [:bz] 2012-10-09 08:08:34 PDT
Comment on attachment 669535 [details] [diff] [review]
patch

r=me

I hate the web.  :(
Comment 2 Gervase Markham [:gerv] 2012-10-10 02:16:27 PDT
Comment on attachment 669535 [details] [diff] [review]
patch

r=gerv, echoing bz's opinion. If it were possible to check for cookies beginning with "MoodleSession" rather than containing it, that would be even better. I believe JS now has a startsWith?

Can you quickly outline what you did to test this?

Gerv
Comment 3 Dão Gottwald [:dao] 2012-10-10 02:23:26 PDT
(In reply to Gervase Markham [:gerv] from comment #2)
> Comment on attachment 669535 [details] [diff] [review]
> patch
> 
> r=gerv, echoing bz's opinion. If it were possible to check for cookies
> beginning with "MoodleSession" rather than containing it, that would be even
> better. I believe JS now has a startsWith?

The Cookie header contains multiple cookies here, so it's more complicated than that.

> Can you quickly outline what you did to test this?

I loaded http://demo.moodle.net/user/edit.php?id=5&course=1 and logged in as admin.
Comment 4 Gervase Markham [:gerv] 2012-10-10 02:26:55 PDT
OK, fair enough on the cookie name.

Gerv
Comment 5 :Felipe Gomes (needinfo me!) 2012-10-10 03:47:16 PDT
Comment on attachment 669535 [details] [diff] [review]
patch

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

Looks good to me.  I'd prefer having "UserAgentOverrides.init();" + the added block in a separate function to keep OnProfileStartup clean, but it's up to you.
Better yet I think would be to have this specific knowledge about Moodle (and other future cases?) in UserAgentOverrides.jsm instead of nsBrowserGlue, but I assume you did it this way to keep this Desktop-only.
Comment 6 Dão Gottwald [:dao] 2012-10-10 05:52:12 PDT
Created attachment 669933 [details] [diff] [review]
patch v2

(In reply to :Felipe Gomes from comment #5)
> Looks good to me.  I'd prefer having "UserAgentOverrides.init();" + the
> added block in a separate function to keep OnProfileStartup clean, but it's
> up to you.

done

> Better yet I think would be to have this specific knowledge about Moodle
> (and other future cases?) in UserAgentOverrides.jsm instead of
> nsBrowserGlue, but I assume you did it this way to keep this Desktop-only.

exactly
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-10 16:53:46 PDT
moving tracking to this bug, removing it from 797703
Comment 9 Ed Morley [:emorley] 2012-10-11 07:02:15 PDT
https://hg.mozilla.org/mozilla-central/rev/bead66674f12
Comment 10 Dão Gottwald [:dao] 2012-10-11 08:02:09 PDT
Comment on attachment 669933 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 588909
User impact if declined: plain textarea instead of rich text editor on moodle (worse user experience)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): reasonably low risk, and the User Agent override for moodle can be disabled via a pref
String or UUID changes made by this patch: none
Comment 11 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-10-11 22:05:03 PDT
Is this feature really need?
The previous similar patch supporting aol.com is even so, I don't think these specified patch is good way.
These approach is domain specific. If this approach is recognized,  other sites in the world have the right to get a same support.

If we need to land this support necessarily, we should take more versatile method (e.g. site specfic option).
Comment 12 Dão Gottwald [:dao] 2012-10-12 03:08:58 PDT
(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #11)
> Is this feature really need?

Yes, see the discussion in bug 797703. We expect that many Moodle instances won't be updated soon and we don't want a worse experience on Moodle for Firefox users.
Comment 13 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-10-12 08:20:44 PDT
(In reply to Dão Gottwald [:dao] from comment #12)
> Yes, see the discussion in bug 797703. We expect that many Moodle instances
> won't be updated soon and we don't want a worse experience on Moodle for
> Firefox users.

OK. I understand the how and why.

However, I don't like this approach...... Even if it's a temporary treatment, this approach is special treatment for a specified site. It may be not browser's business as a Web-platform.
Comment 14 Tim Hunt 2012-10-12 08:30:45 PDT
Moodle is a web application that lots of universities (and other people) install on their own servers. So, adding a work-around for Moodle is like adding a work-around for Drupal or Wordpress. It is not like adding a work-around for Twitter or Facebook.
Comment 15 Boris Zbarsky [:bz] 2012-10-12 08:31:32 PDT
No one _likes_ this approach.

Do feel free to suggest a better one if you have one....
Comment 16 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-10-12 09:47:01 PDT
Tim, Boris, Thank you for your accounts.

At comment 13, I did not have a mind to go against this approach strongly.
I apologize that my comment dragged up the discussion.
Comment 17 Alex Keybl [:akeybl] 2012-10-12 15:05:43 PDT
Comment on attachment 669933 [details] [diff] [review]
patch v2

Thanks guys - I know this isn't the perfect solution, but it will definitely make our users have a much better experience.
Comment 19 Dan Poltawski 2012-11-01 22:09:11 PDT
A big thanks from Moodle for implementing this workaround. We've integrated a fix for the underlying problem in Moodle (http://tracker.moodle.org/browse/MDL-35469), but it'll take sites some time to update with this fix.
Comment 20 Virgil Dicu [:virgil] [QA] 2012-11-12 05:57:58 PST
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0

Setting to verified in F17 beta 5. Checked on Windows 7, Mac OS 10.7, Ubuntu 12.04. 
Rich text editor displayed as expected and functional on http://demo.moodle.net/user/edit.php?id=5&course=1

The site's layout is broken in Firefox however. Logged bug 810828.
Comment 21 albert.gasset 2012-11-23 03:16:02 PST
This change breaks another feature, linking Moodle with Mahara using MNet: http://docs.moodle.org/23/en/MNet 

MNet allows users of a Moodle site to roam to a Mahara site. The atuhentication process checks if the User Agent string received by both sites are equal to allow access.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-23 13:06:17 PST
(In reply to albert.gasset from comment #21)
> This change breaks another feature, linking Moodle with Mahara using MNet:
> http://docs.moodle.org/23/en/MNet 
> 
> MNet allows users of a Moodle site to roam to a Mahara site. The
> atuhentication process checks if the User Agent string received by both
> sites are equal to allow access.

That sounds like tech evangelism to Mahara then - as it is a website and not an application, please file a separate bug for Tech Evangelism.
Comment 23 albert.gasset 2012-11-26 00:24:07 PST
Mahara is an application, it's an open source portfolio system that is commonly used with Moodle. This is causing a serious problem for its users: https://bugs.launchpad.net/mahara/+bug/1082416?comments=all

I understand this workaround was implement to avoid trouble for some users, but for other users this is even worse. Why not fix the problem at the right place (Moodle), to avoid the risk of more side effects?

(In reply to Lukas Blakk [:lsblakk] from comment #22)
> (In reply to albert.gasset from comment #21)
> > This change breaks another feature, linking Moodle with Mahara using MNet:
> > http://docs.moodle.org/23/en/MNet 
> > 
> > MNet allows users of a Moodle site to roam to a Mahara site. The
> > atuhentication process checks if the User Agent string received by both
> > sites are equal to allow access.
> 
> That sounds like tech evangelism to Mahara then - as it is a website and not
> an application, please file a separate bug for Tech Evangelism.
Comment 24 Dan Poltawski 2012-11-26 00:43:20 PST
Hi Albert,

In Moodle, we have now fixed this ( http://tracker.moodle.org/browse/MDL-35469 ), however i'm afraid its not likely at all that all Moodle sites will be upgraded with this fix.
Comment 25 Gervase Markham [:gerv] 2012-11-26 02:24:05 PST
(In reply to albert.gasset from comment #21)
> MNet allows users of a Moodle site to roam to a Mahara site. The
> atuhentication process checks if the User Agent string received by both
> sites are equal to allow access.

The obvious question is: what on earth for? Given that you have a proper SSO system, this provides precisely zero added security.

If it's the considered opinion of the Moodle leadership that we should remove the workaround, we will. You have more info than us to make the decision on which problem is worse.

Gerv
Comment 26 Boris Zbarsky [:bz] 2012-11-26 08:36:27 PST
Can we simply send the same cooked-up UA to Mahara?  Can we detect when we're dealing with it?
Comment 27 Matthew N. [:MattN] (behind on reviews) 2012-11-26 16:48:18 PST
I filed bug 815446 for the MNet SSO fallout.
Comment 28 Ed Morley [:emorley] 2012-11-27 13:08:07 PST
Partly backed out by bug 815743; rest needs cleanup -> reopening.
Comment 29 Dão Gottwald [:dao] 2012-12-03 07:18:53 PST
(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #28)
> Partly backed out by bug 815743; rest needs cleanup -> reopening.

Could you be more specific? What's the rest and what kind of cleanup is needed?

Note You need to log in before you can comment on or make changes to this bug.