[Meta] Update / Refactor JavaScript as a result of upgrading to jQuery 1.7 [#1332066]

Comment #1

cosmicdreams CreditAttribution: cosmicdreams commented 4 November 2011 at 16:06

Status:

Active

» Postponed

Postponed until #1328900: Update to jQuery 1.7 is in.

Log in or register to post comments

Comment #2

rupl

he/him

English

CreditAttribution: rupl commented 6 November 2011 at 20:01

I found an issue after applying the patch in #1328900: Update to jQuery 1.7. When adding new fields to any content type, selecting an option from the Field drop-down does not cause the Widget drop-down to become enabled. Since the drop-down is always disabled, it prevents fields from being saved.

To reproduce:

Upgrade jQuery to 1.7 using the other issue's patch.
Navigate to admin/structure/types/manage/page/fields
Fill out the new field form row. Notice how tab completion skips "widget" column's form element. Inspect element using your browser dev tool of choice. Notice that element still contains disabled="disabled".
Submit form. Validation error occurrs asking user to fill out widget form item.

Edit: Rob has fixed this issue in http://drupal.org/node/1328900#comment-5210566

Log in or register to post comments

Comment #3

RobLoach

he/him

CreditAttribution: RobLoach commented 14 November 2011 at 16:47

Title:

Update / Refactor JavaScript as a result of upgrading to jQuery 1.7

» [Meta] Update / Refactor JavaScript as a result of upgrading to jQuery 1.7

Changing topic focus to point to other issues to refactor the JavaScript.

Log in or register to post comments

Comment #4

catch

he/him

English

CreditAttribution: catch commented 15 November 2011 at 02:21

Priority:	Normal	» Major
Status:	Postponed	» Active

I've just committed #1328900: Update to jQuery 1.7.

Log in or register to post comments

Comment #5

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 14:39

Excellent! Ladies and gentlemen we could do a lot of work with this patch. Here's a list of scripts in Drupal that use the bind event:

core/misc/vertical-tabs.js
core/misc/jquery.form.js
core/misc/form.js
core/misc/ajax.js
core/misc/collapse.js
core/misc/tableheader.js
core/misc/textarea.js
core/misc/tabledrag.js
core/misc/states.js
core/misc/machine-name.js
core/modules/user/user.js
core/modules/user/user.permissions.js
core/modules/contextual/contextual.js
core/modules/filter/filter.js
core/modules/filter/filter.admin.js
core/modules/overlay/overlay-child.js
core/modules/overlay/overlay-parent.js
core/modules/color/color.js
core/modules/file/file.js
core/modules/system/system.js
core/modules/field_ui/field_ui.js

As per http://blog.jquery.com/2011/11/03/jquery-1-7-released/ it is now recommended that we use the newly created .on and .off events instead of .bind, .delegate and .unbind.

That will require us to go through "every" script to update it's code. Perhaps this is a simple find-and-replace perhaps not. This change highlights the need for test coverage on all of our javascript. Does anyone know if we have that or what it will take to have that?

Log in or register to post comments

Comment #5.0

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 14:39

Issue summary:

View changes

added another issue to watch for this meta topic

Log in or register to post comments

Comment #6

aspilicious CreditAttribution: aspilicious commented 15 November 2011 at 12:08

#1342198: Use .on and .off instead of .bind, .unbind and .delegate is done I guess just needs testing.

(added to the summary too)

Log in or register to post comments

Comment #6.0

aspilicious CreditAttribution: aspilicious commented 15 November 2011 at 12:08

Issue summary:

View changes

Updated issue summary.

Log in or register to post comments

Comment #7

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 14:38

Thanks aspillicious, that seems to be the most major change for version of jQuery. But also, we shouldn't use isNaN anymore as it will be going away soon.

The only Drupal-authored JavaScript this effects is overlay's overlay-parent.js. Created an issue for it #1342336: Remove isNaN() from our javascripts and add it to the summary.

Log in or register to post comments

Comment #7.0

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 14:38

Issue summary:

View changes

Added issue

Log in or register to post comments

Comment #8

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 14:58

Also created this issue #1342370: Can we improve our JavaScript by using $.Callbacks ? that could be a such win for readability (and maybe performance) for our JavaScript.

Log in or register to post comments

Comment #9

aspilicious CreditAttribution: aspilicious commented 15 November 2011 at 15:53

We should also look at changes in jquery 1.5 and 1.6

1.5 is about a rewrite of ajax in jquery: http://blog.jquery.com/2011/01/31/jquery-15-released/
1.6 is about attr vs prop http://blog.jquery.com/2011/05/03/jquery-16-released/

Log in or register to post comments

Comment #9.0

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 16:40

Issue summary:

View changes

Added a new issue to point to an area of interested for the refactoring efforts.

Log in or register to post comments

Comment #10

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 16:50

Oh? Very interesting... I'll help create issues if they are needed.

Edit: changed description to include references to source material

Log in or register to post comments

Comment #10.0

cosmicdreams CreditAttribution: cosmicdreams commented 15 November 2011 at 16:50

Issue summary:

View changes

Changed issue description to include more references to things that are in scope.

Log in or register to post comments

Comment #11

cosmicdreams CreditAttribution: cosmicdreams commented 14 December 2011 at 03:38

I'd also like to keep these performance considerations in mind during the refactor:
http://24ways.org/2011/your-jquery-now-with-less-suck

For now, I just wanted to post this link to the community. Later, when I have time, I'll review the javascript we currently have and try to find areas where we could improve jQuery performance using the techniques outlined in this article. (but I suspect I won't find any)

Log in or register to post comments

Comment #12

klonos

he/him

English

90% Melbourne, Australia - 10% Larissa, Greece

CreditAttribution: klonos commented 14 December 2011 at 07:26

I went only half-way through the linked article and I'd like to add this as a note for when we decide to optimize our js:

This comes from the selector optimization section of that article and the $("#id p"); vs $("#id").find("p"); part specifically, but I'm merely using it as an example in order to make a point... I'm sure that this will prove to be right, but when people not aware of these optimization "tricks" revisit these optimized parts, they might choose to switch them back to the shorter version (Less Code™). So, perhaps we should open a separate issue for that (set to postponed till we have time to work on it) that has as its tasks to:

- document these best practices somewhere in d.o (give proper credits to the linked/original article & its authors), perhaps as official coding standards too. Reason is that information around the internet tents to get mis/dis-placed over time, so we should keep a "local" copy - just in case. Plus, perhaps more articles might come up over time from various other sources. We need to keep all these in a common place. Coding standards sound like that place to me.
- stress that whenever these optimizations happen, they should be accompanied by a note/comment linking to the d.o article stating the reason why the certain way of coding was chosen (so we avoid people reverting to other non-optimized code - for whatever reason).