1014609 - Remove dependency on jquery

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[aleth [:aleth]]

Thunderbird currently uses jquery for the faceted search results and the account provisioner:

http://mxr.mozilla.org/comm-central/search?string=jquery.js
http://dxr.mozilla.org/comm-central/search?q=path%3Ajquery&redirect=true

It would be much better to replace this with vanilla JS.

Comment 1

[aleth [:aleth]]

Here's a nicer query, courtesy of clokep ;) http://dxr.mozilla.org/comm-central/search?q=regexp%3Ajquery.%2B\.js&case=false

Comment 2

[Magnus Melin [:mkmelin]]

It has its sides, but changing it would be a fair amount of work + jquery is ubiquitous for web development so does make it easier to get started for someone coming from that background.

Comment 3

[:aceman]

And harder to use for someone without that background. It also floods the console with tons of strict mode warnings.

Comment 4

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin from comment #2)
> It has its sides, but changing it would be a fair amount of work
That's why Thunderbird is open source and if someone is willing to do it, we should happily accept it!

http://dxr.mozilla.org/comm-central/search?q=regexp%3Ajquery.*\.js&redirect=true is the right search by the way, had to switch the + to a *.

Looking through the gloda stuff it's used for only a few things:
- hide
- show
- slideUp
- resize
- removeAttr
- slideDown
- attr

Most of that looks very easy to replace with vanilla JS.

Comment 5

[Florian Quèze [:florian]]

Exciting suggestion! :-)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I just found this issue.
I was suspecting an intermittent failure (bug 910293) is caused by JQuery bug. So I hope that orange will be something change if JQuery is removed.

Does anyone have an in-progress patch?

Comment 7

[Patrick Cloke [:clokep]]

I don't believe anyone has a WIP patch. (I certainly don't!)

Comment 8

[aleth [:aleth]]

If anyone *is* currently working on this, please assign yourself to the bug ;)

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Remove jquery.scrollTo.js [checked in]

There is no use of jquery.scrollTo.js

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Remove jquery method in mozill tests [checked in]

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3-a: Remove jquery from gloda stuff (glodaFacetView.js) [checked in]

I don't know who is a proper person to review this.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3-b: Remove jquery from gloda stuff (glodaFacetBindings.xml) [checked in]

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3-c: Remove jquery.js from gloda stuff (glodaFacetView.xhtml) [checked in]

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Remove jquery.tmpl.js

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part X: Remove jquery in accountProvisioner.js. (WIP)

This patch is still in progress and broken!

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Try server results with these patches are fine:

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=2cf677e2912f

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Remove jquery.tmpl.js

I am sorry the previous patch has a bug. (I actually thought the bug in Part X patch). I did not understand how template element works.

Comment 18

[aleth [:aleth]]

Comment on attachment 8532857 [details] [diff] [review]
Part X: Remove jquery in accountProvisioner.js. (WIP)

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +118,5 @@
>     */
>    searchEnabled: function EAP_searchEnabled(aVal) {
> +    document.getElementById("name").disabled = !aVal;
> +    [].forEach.call(document.querySelectorAll(".providerCheckbox"), function(node) {
> +      node.disabled = !aVal;

It's nicer to do
for (let node of document.querySelectorAll(...)) node.disabled = !aVal;

@@ +148,5 @@
> +      let searchEngineWrap = document.getElementById("search_engine_wrap");
> +      let searchEngineCheck = document.getElementById("search_engine_check");
> +      searchEngineWrap.style.display = "block";
> +      searchEngineWrap.addEventListener("click", function() {
> +        searchEngineCheck.click();

Why not put the |let searchEngineCheck = document.getElementById("search_engine_check");| inside the function?

Also, I don't think the click() function is defined anywhere.

Comment 19

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #18)
> Also, I don't think the click() function is defined anywhere.

Oh sorry, I guess you're using https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement.click

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to aleth [:aleth] from comment #18)
> ::: mail/components/newmailaccount/content/accountProvisioner.js
> @@ +118,5 @@
> >     */
> >    searchEnabled: function EAP_searchEnabled(aVal) {
> > +    document.getElementById("name").disabled = !aVal;
> > +    [].forEach.call(document.querySelectorAll(".providerCheckbox"), function(node) {
> > +      node.disabled = !aVal;
> 
> It's nicer to do
> for (let node of document.querySelectorAll(...)) node.disabled = !aVal;

Thanks! I will do it.

> @@ +148,5 @@
> > +      let searchEngineWrap = document.getElementById("search_engine_wrap");
> > +      let searchEngineCheck = document.getElementById("search_engine_check");
> > +      searchEngineWrap.style.display = "block";
> > +      searchEngineWrap.addEventListener("click", function() {
> > +        searchEngineCheck.click();
> 
> Why not put the |let searchEngineCheck =
> document.getElementById("search_engine_check");| inside the function?

That's because #search_engine_check element is also used a few lines below.

Comment 21

[Richard Marti (:Paenglab)]

Comment on attachment 8532853 [details] [diff] [review]
Part 3-a: Remove jquery from gloda stuff (glodaFacetView.js) [checked in]

The CSS code looks good, but my JS knowledge not good enough to review this. I'm taking this review over to Blake as he has already a request in this bug. Magnus would also be a good candidate.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8532854 [details] [diff] [review]
Part 3-b: Remove jquery from gloda stuff (glodaFacetBindings.xml) [checked in]

Again, Blake is a much more better candidate for such reviews.

Comment 23

[Josiah Bruner [:jsbruner]]

Blake said he was going to be extremely busy for a couple weeks, so we may want to pass those reviews on. I could take 1, 3-A and 3-B, and perhaps Magnus could review part 4?

So Hiro, if you'd like to land this sooner, feel free to pass those reviews. If not, then great! We'll just wait for Blake.

Comment 24

[:aceman]

Great stuff Hiro!

Comment 25

[aleth [:aleth]]

Comment on attachment 8532853 [details] [diff] [review]
Part 3-a: Remove jquery from gloda stuff (glodaFacetView.js) [checked in]

Yes, many thanks Hiro for tackling this!

Comment 26

[aleth [:aleth]]

Comment on attachment 8532850 [details] [diff] [review]
Part 1: Remove jquery.scrollTo.js [checked in]

This should be a quick review, but I'm not a peer for this part of /mail.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8532901 [details] [diff] [review]
Part 4: Remove jquery.tmpl.js

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

Other than the comment, it looks good.  Thank you for the patch!  :)

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +777,3 @@
>  
> +        group.append(result);
> +        // If we got here, then we were able to successfully render the

Can this ever fail now?  If not, let's remove the first sentence of this comment.

Comment 28

[aleth [:aleth]]

It seems this is all r+ now apart from the WIP patch :-)

Comment 29

[Magnus Melin [:mkmelin]]

We should get finished patches landed here. Some of the have nits to address.

Comment 30

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/6e421c79d586
https://hg.mozilla.org/comm-central/rev/90852d005f77
https://hg.mozilla.org/comm-central/rev/abbb61a1b23f
https://hg.mozilla.org/comm-central/rev/0756788b54d4
https://hg.mozilla.org/comm-central/rev/57fa955e015e

Comment 31

[aleth [:aleth]]

Attachment: Part 4: Remove jquery.tmpl.js, unbitrotted, nit fixed [checked in]

(In reply to Blake Winton (:bwinton) from comment #27)
> Comment on attachment 8532901 [details] [diff] [review]
> Part 4: Remove jquery.tmpl.js
> ::: mail/components/newmailaccount/content/accountProvisioner.js
> @@ +777,3 @@
> >  
> > +        group.append(result);
> > +        // If we got here, then we were able to successfully render the
> 
> Can this ever fail now?  If not, let's remove the first sentence of this
> comment.

Looking at bug 700536 (where the try/catch was added), it appears this was done as jQuery had problems if addresses contained e.g. something that could be interpreted as invalid XML. This patch does a straightforward string replace, so there should be no issues.

Comment 32

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/349f3470bcce

Comment 33

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/f7e2646bf893

Comment 34

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=110ad4f6dd12

Comment 35

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20e93c15f7d9

Comment 36

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=677a28bf08dd

Comment 37

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd65f6a59355

Comment 38

[aleth [:aleth]]

Attachment: Part 6: Remove jquery itself

Comment 39

[aleth [:aleth]]

Attachment: Part 5: Remove jquery in accountProvisioner.js

Comment 40

[aleth [:aleth]]

Attachment: Part 5: Remove jquery in accountProvisioner.js: fix corresponding mozmill test

Comment 41

[Kent James (:rkent)]

Since this bug was first created, a lot has happened in the Mozilla world. Announcements of the deprecation of XUL will ultimately force Thunderbird to react, and that reaction is likely to take the form of moving much of the Thunderbird UI to HTML. In the process, the typical path would be to add into the core code various js libraries that support modern UI interfaces in UI. That trend is the opposite of what is being done in this bug.

See http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/loop/content/shared/libs/react-0.12.2.js as an example of recent Mozilla code that is moving in that direction.

So though I am not opposing the path of this current bug, which started over a year ago, just a caution for the future: we may need to reverse the trend being set by this bug at some point, and go back to adding complex js libraries into the Thunderbird code base, including jquery.

Comment 42

[Patrick Cloke [:clokep]]

(In reply to Kent James (:rkent) from comment #41)
> Since this bug was first created, a lot has happened in the Mozilla world.
> Announcements of the deprecation of XUL will ultimately force Thunderbird to
> react, and that reaction is likely to take the form of moving much of the
> Thunderbird UI to HTML. In the process, the typical path would be to add
> into the core code various js libraries that support modern UI interfaces in
> UI. That trend is the opposite of what is being done in this bug.

This is removing an old version of jQuery. When we switch to doing things in HTML we'll need to make decisions about how we want to do it. Instead of just using whatever old stuff is left in our code base.

> So though I am not opposing the path of this current bug, which started over
> a year ago, just a caution for the future: we may need to reverse the trend
> being set by this bug at some point, and go back to adding complex js
> libraries into the Thunderbird code base, including jquery.

I disagree. We need to consider starting to use HTML; not using bloated JavaScript libraries "just because". Regardless this library throws a ton of warnings in the error console and has little value. All of its calls are easily replaced with vanilla JavaScript.

Overall: I think removing this still has value, especially because the work is done already.

Comment 43

[Kent James (:rkent)]

(In reply to Patrick Cloke [:clokep] from comment #42)
> 
> Overall: I think removing this still has value, especially because the work
> is done already.

Yes that is fine, and I generally agree with all of your points. I only added the comment here, as if we need to reverse this in the future (which may or may not be the right thing to do) I don't want us pointing to the bug as a difficult-to-change precedent implying we are totally opposed to external libraries.

Comment 44

[aleth [:aleth]]

(In reply to Kent James (:rkent) from comment #41)
> Since this bug was first created, a lot has happened in the Mozilla world.
> Announcements of the deprecation of XUL will ultimately force Thunderbird to
> react, and that reaction is likely to take the form of moving much of the
> Thunderbird UI to HTML. In the process, the typical path would be to add
> into the core code various js libraries that support modern UI interfaces in
> UI. That trend is the opposite of what is being done in this bug.
>
> See
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/loop/
> content/shared/libs/react-0.12.2.js as an example of recent Mozilla code
> that is moving in that direction.
> 
> So though I am not opposing the path of this current bug, which started over
> a year ago, just a caution for the future: we may need to reverse the trend
> being set by this bug at some point, and go back to adding complex js
> libraries into the Thunderbird code base, including jquery.

Your points make a lot of sense, but it seems you've looked at the proposed change from a different altitude than I did. When looking closer at the patch, I see:

- Part of the changes in the patch move away from the XUL box model to CSS flexbox. This is very much in line with "killing XUL".

- The account provider UI was written as (almost) HTML from the start. Jquery was used at the time not least because it made animations easy. In 2015, these features are built into the web (CSS transitions). So moving towards web standards means being able to drop the additional layer. To use webdev language, you could consider the use of jquery here a polyfill of sorts for HTML5 and CSS3, which is now obsolete.

- Choosing to use any particular library means having to maintain that library in the future, and keep it up to date. This too often gets forgotten and is clearly something which did not happen here. Conversely, vanilla JS and HTML are much more robust.

- Using a library has another tradeoff: it means adding another layer of code which is effectively a black box and can hide bugs. There's an example of this here: Hiding a button with jquery doesn't actually disable it, which means that mozmill could "click" a button that was hidden. So a test was passing which should have failed. (In this case it turns out the bug was in the test code.)

- Fwiw, Loop has also just dropped their dependency on jquery (bug 1186396).

(In reply to Kent James (:rkent) from comment #43)
> Yes that is fine, and I generally agree with all of your points. I only
> added the comment here, as if we need to reverse this in the future (which
> may or may not be the right thing to do) I don't want us pointing to the bug
> as a difficult-to-change precedent implying we are totally opposed to
> external libraries.

Sure, for every specific case, whether to use a particular library or not should be evaluated on its own merits.

Comment 45

[Richard Marti (:Paenglab)]

Attachment: dialog.png

Strange on Linux it shows email addresses to buy, on Win 10 it can't find any.

But thanks to this I found the in the image visible red error text doesn't appear. There is a missing opacity: 1 to revert the rule

#notifications .error {
  opacity: 0;
}

Comment 46

[Richard Marti (:Paenglab)]

Comment on attachment 8667217 [details] [diff] [review]
Part 5: Remove jquery in accountProvisioner.js

Setting ui-r- because of the issue reported in the previous comment.

Comment 47

[aleth [:aleth]]

Attachment: Part 5: Remove jquery in accountProvisioner.js

Comment 48

[aleth [:aleth]]

Comment on attachment 8667564 [details] [diff] [review]
Part 5: Remove jquery in accountProvisioner.js

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

> But thanks to this I found the in the image visible red error text doesn't appear. There is a missing opacity: 1 to revert the rule...

Thanks for spotting this! It was actually due to the transition not happening due to display=none being involved. Forcing a relayout of that element so it picks up the display=block before changing the opacity to 1 makes the transition work.

Turns out the error string also had a typo in it, which seems to have gone unnoticed for years.

> Strange on Linux it shows email addresses to buy, on Win 10 it can't find any.

I noticed during testing that some providers don't seem to like multiple requests for the same search string, maybe that's what caused it.

Comment 49

[Richard Marti (:Paenglab)]

Comment on attachment 8667564 [details] [diff] [review]
Part 5: Remove jquery in accountProvisioner.js

Today the search on none of my systems (also with TB 38) gives a mail address. But all other I've seen works as before.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 8667564 [details] [diff] [review]
Part 5: Remove jquery in accountProvisioner.js

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

Looks good, just some nits. r=mkmelin with those fixed

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +118,5 @@
>     */
>    searchEnabled: function EAP_searchEnabled(aVal) {
> +    document.getElementById("name").disabled = !aVal;
> +    for (let node of document.querySelectorAll(".providerCheckbox"))
> +      node.disabled = !aVal;

please always have braces even for one-one loops

@@ +128,5 @@
>     */
>    spinning: function EAP_spinning(aVal) {
> +    let display = aVal ? "block" : "none";
> +    for (let node of document.querySelectorAll("#notifications .spinner"))
> +      node.style.display = display;

here too

@@ +405,5 @@
>     * name to the suggestFromName service.
>     */
>    onSearchSubmit: function EAP_onSearchSubmit() {
> +    for (let node of document.getElementById("notifications").children)
> +      node.style.display = "none";

braces

@@ +441,5 @@
> +    request.onload = function () {
> +      let data;
> +      try {
> +        data = JSON.parse(request.responseText);
> +      } catch(e) {};

should show error and bail out if we end up in the catch

@@ +507,5 @@
>      // Wait for the handler to close us.
>      EmailAccountProvisioner.spinning(true);
>      EmailAccountProvisioner.searchEnabled(false);
> +    for (let node of document.querySelectorAll("#notifications > :not(.spinner)"))
> +      node.style.display = "none";

braces

@@ +539,5 @@
> +    request.onload = function() {
> +      let data;
> +      try {
> +        data = JSON.parse(request.responseText);
> +      } catch(e) {};

here too

@@ +545,5 @@
> +      EmailAccountProvisioner.populateProviderList(data);
> +    };
> +    request.onerror = () => {
> +      // Ugh, we couldn't get the JSON file.  Maybe we're not online.  Or maybe
> +      // the server is down, or the file isn't being served.  Regardless, if

(just moved but) lots of extra spaces in between. can we remove them while we're here

@@ +553,5 @@
> +                          RETRY_TIMEOUT);
> +      EmailAccountProvisioner._loadingProviders = false;
> +      EmailAccountProvisioner.beOffline();
> +      gLog.error("Something went wrong loading the provider list JSON file. "
> +                 + "Going into offline mode.");

the convertion is to always have + on the previous row

@@ +657,3 @@
>  
>        if (supportsSomeUserLang) {
> +        providerCheckbox.setAttribute('checked', 'true');

for consistency, use double quotes

@@ +672,1 @@
>  

braces

@@ +742,5 @@
>     * Make the search pane a little bit taller, and the existing account
>     * pane a little bit shorter.
>     */
>    expandSearchPane: function() {
> +    /* This doesn't do anything useful and is broken.

let's remove it then

@@ +755,5 @@
> +      // Some of these selectors don't even have any hits.
> +      document.getElementById("providers").classList.add("showWithFade");
> +      document.querySelector("#content .description").classList.add("showWithFade");
> +      for (let node of document.querySelectorAll(".tinyheader .title"))
> +        node.classList.add("showWithFade");

braces

@@ +769,5 @@
>     * we might want to show something a bit more descriptive.
>     */
>    showSearchError: function() {
> +    for (let node of document.getElementById("notifications").children)
> +      node.style.display = "none";

braces

@@ +893,3 @@
>            result.classList.add("extra");
> +          for (let address of result.querySelectorAll(".address"))
> +            address.classList.add("hideWithFade");

braces

Comment 51

[aleth [:aleth]]

Comment on attachment 8667564 [details] [diff] [review]
Part 5: Remove jquery in accountProvisioner.js

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +441,5 @@
> +    request.onload = function () {
> +      let data;
> +      try {
> +        data = JSON.parse(request.responseText);
> +      } catch(e) {};

No, this is correct. onSearchResults already handles the case where data is undefined.

Comment 52

[aleth [:aleth]]

Attachment: Part 5: Remove jquery in accountProvisioner.js

Comment 53

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/32eb613ad2eb74dab38722910b4e4e2b08ee780e
Bug 1014609 - Part 5: Remove jquery in accountProvisioner.js. r=mkmelin,ui-r=Paenglab

https://hg.mozilla.org/comm-central/rev/c5401915af893260262edddd66921570370fe3c3
Bug 1014609 - Part 5: Remove jquery in accountProvisioner.js: fix corresponding mozmill test. r=mkmelin

https://hg.mozilla.org/comm-central/rev/180bafc8f18cc4bae161fa14b192d356b9284bd8
Bug 1014609 - Part 6: Remove jquery itself. r=mkmelin a=aleth