1119229 - Remove onsync(to|from)preference attributes from in-content prefs

Status: NEW

(Firefox :: Settings UI, defect)

Description

[:Gijs (he/him)]

See bug 1016300 comment 13/14. AFAICT we could fix preferences.xml to always synthesize the events or use the event listener manager to check if the element has event handlers.

Comment 1

[nithin[:lazybug]]

i would like to work on this bug

Comment 2

[nithin[:lazybug]]

Since I'm new to mozilla community, i need some help in getting started. can you please explain me the bug.

Comment 3

[:Gijs (he/him)]

(In reply to nithin from comment #2)
> Since I'm new to mozilla community, i need some help in getting started. can
> you please explain me the bug.

Hey nithin,

Great!

Are you familiar with JavaScript and DOM events? If so, you could have a look at fixing this bug. Please see https://developer.mozilla.org/en-US/docs/Introduction about how to get a Firefox build set up. That will be the first thing to sort out. You can use the "Need more information" checkbox and field to ping me again once you've done that.
Basically, you will need to change the XUL files under browser/components/preferences/in-content/ and move the inline "onsyncfrompreference" and "onsynctopreference" event handlers into the JS files that accompany the XUL files (they have the same names, apart from the extensions). You'll also need to update toolkit/content/widgets/preferences.xml to not make the event dispatch of these events conditional on the attributes being there.

If that sounds like it's a bit difficult because you're not that familiar with events and JS, that's not a problem, but I would suggest you give bug 1098517 a shot first, as it will be a bit simpler to get started with. You'll still need a Firefox build though - once you've gotten a build, comment on the bug and Matt or I can help you further there.

Comment 4

[:Gijs (he/him)]

Oh, and if you have any questions about how to build Firefox itself, it's probably best to ask in the #introduction chat room on irc.mozilla.org.

Thanks!

Comment 5

[:Gijs (he/him)]

Nithin, are you still working on this? Do you need more help getting going?

Comment 6

[nithin[:lazybug]]

i thought i will start working on this once i have finished the bug you referred https://bugzilla.mozilla.org/show_bug.cgi?id=1098517 . now i have submitted a patch for that(yet to be reviewed ), i will start working on this bug.

Comment 7

[nithin[:lazybug]]

Attachment: onsync_v1.patch

using window.addEventListener(){}

Comment 8

[:Gijs (he/him)]

Comment on attachment 8566432 [details] [diff] [review]
onsync_v1.patch

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

::: browser/components/preferences/in-content/main.js
@@ +138,5 @@
>      Components.classes["@mozilla.org/observer-service;1"]
>                .getService(Components.interfaces.nsIObserverService)
>                .notifyObservers(window, "main-pane-loaded", null);
> +
> +   window.addEventListener("onsyncfrompreference", function() {

I don't understand why you're adding an event listener to the window?

@@ +139,5 @@
>                .getService(Components.interfaces.nsIObserverService)
>                .notifyObservers(window, "main-pane-loaded", null);
> +
> +   window.addEventListener("onsyncfrompreference", function() {
> +      document.getElementById("browserHomePage").addEventListener("onsyncfrompreference", "return gMainPane.syncFromHomePref();");

This should be "syncfrompreference", and the same below, also for "synctopreference". The second parameter to "addEventListener" is a function, not a string, so these should be functions, not strings. Same for all the other calls.

Also, you will need to adapt all the other .xul files in this directory, and probably "languages.xul" in the directory above this one as well.

After that, you will need to change http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#433 and friends so that the relevant events always get fired.

Then you should run the tests for this code (or at least rebuild Firefox's browser/components directory and open the preferences in your updated build) to see if things actually still work... :-)

::: browser/components/preferences/in-content/preferences.xul
@@ -148,5 @@
>          <image class="category-icon"/>
>          <label class="category-name" flex="1">&paneSecurity.title;</label>
>        </richlistitem>
>  
> -#ifdef MOZ_SERVICES_SYNC

You shouldn't remove the #ifdefs here.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Hi Nithin, any update on this?

Comment 10

[nithin[:lazybug]]

im having semester exams. and its GSoC application period, a bit busy. Will fix it as soon as internals are over :)

Comment 11

[nithin[:lazybug]]

Attachment: onsync_v2.patch

it seems working. i don't know how to run tests for this piece of code. so i haven't tested it.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8589858 [details] [diff] [review]
onsync_v2.patch

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

There are a lot of typoes here, and some changes that I'd recommend in preferences.xml (noted below). I would recommend you go through the diff and review it carefully yourself: 

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1119229&attachment=8589858

Then adjust your patch accordingly and run some tests.

Basically, I think you should run at least:

./mach build browser/components toolkit/content

to rebuild, and then:

./mach mochitest-chrome toolkit/content/tests/test_preferences*

(note the *)

and once you have that passing, run:

./mach mochitest-browser browser/components/preferences

If you need help as to how to adjust code/tests in order to get this to pass, please attach an updated patch and needinfo me or ask for "feedback". Either one (not both) will be enough. :-)

::: browser/components/preferences/in-content/privacy.js
@@ +102,5 @@
>      setEventListener("showCookiesButton", "command",
>                       gPrivacyPane.showCookies);
>      setEventListener("clearDataSettings", "command",
>                       gPrivacyPane.showClearPrivateDataSettings);
> +    setEventListener("acceptCookies", "syncfromprefrence",

All of these in this file are typoed.

::: browser/components/preferences/languages.js
@@ +14,5 @@
>    {
> +    function setEventListener(aId, aEventType, aCallback)
> +    {
> +      document.getElementById(aId)
> +              .addEventListener(aEventType, aCallback.bind(gAdvancedPane));

copy-paste mistake? This shouldn't be gAdvancedPane.

@@ +21,4 @@
>      if (!this._availableLanguagesList.length)
>        this._loadAvailableLanguages();
> +    
> +    setEventListener("activeLanguages", "synctoprefrence",

Typo.

@@ +22,5 @@
>        this._loadAvailableLanguages();
> +    
> +    setEventListener("activeLanguages", "synctoprefrence",
> +                     gLanguagesDialog.writeAcceptLanguages);
> +    setEventListener("activeLanguages", "syncfromprefrence",

And another typo.

::: toolkit/content/widgets/preferences.xml
@@ +433,4 @@
>              // Value changed, synthesize an event
>              try {
>                var event = document.createEvent("Events");
> +              event.initEvent("syncfrompreference", true, true); 

Nit: trailing space.

@@ +433,5 @@
>              // Value changed, synthesize an event
>              try {
>                var event = document.createEvent("Events");
> +              event.initEvent("syncfrompreference", true, true); 
> +              aElement.dispatchEvent(event);

I'm pretty sure you should assign to 'rv' here.

@@ +482,1 @@
>              // Value changed, synthesize an event

Please reindent things now that the if statement is gone (ie remove two spaces in front of the lines that used to be in the if block).

@@ +482,5 @@
>              // Value changed, synthesize an event
>              try {
>                var event = document.createEvent("Events");
>                event.initEvent("synctopreference", true, true);
> +              aElement.dispatchEvent(event);

You should check the result of this, too.

@@ +773,5 @@
>            // Panel loaded, synthesize a load event. 
>            try {
>              var event = document.createEvent("Events");
>              event.initEvent(aEventName, true, true);
> +

Please undo this extra newline in the unrelated code.

Comment 13

[:Gijs (he/him)]

Hey nithin, are you still working on this? Do you need more help here? Please don't hesitate to ask.

Comment 14

[nithin[:lazybug]]

no, i am not working on this anymore. sorry for the delay.

Comment 15

[:Gijs (he/him)]

Rakhi will take this! :-)

Comment 16

[:Gijs (he/him)]

While Rakhi did a bunch of good work in this bug, we ultimately realized that no event dispatch is currently in use. Instead, the attributes get 'eval'd to provide a return value (other than just a plain true/false rv, so not equivalent to e.g. event.isDefaultPrevented) and so converting these to use "proper" events is quite involved, and we won't be pursuing this further for the moment. As a result, it probably also isn't a good mentored bug. Unassigning Rakhi and removing myself as a mentor to clear that up.

Rakhi, would you mind attaching a diff of these changes in case we decide to work on this again in the future?

Comment 17

[Rakhi(:rakhisharma)]

Sure Gijs. Here is the diff in case anyone wants to work on this bug- https://pastebin.mozilla.org/8890901
Thanks :)

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 8890901.txt

Adding Rakhi's patch as an attachment to make sure it is persisted.