Problem/Motivation
Convert type selectors to be compatible with with jQuery native-API selector
Proposed resolution
Follow this replacement pattern:
$( ":button" ) -> $( "button, input[type='button']" ).
$( ":checkbox" ) -> $( "[type=checkbox]" )
$( ":file" ) -> $( "[type="file"]" )
$( ":image" ) -> $( "[type="image"]" )
$( ":password" ) -> $( "[type=password]" )
$( ":radio" ) -> $( "[type=radio]" )
$( ":reset" ) -> $( "[type=reset]" )
when using :hidden to select elements, first select the elements using a pure CSS selector, then use .filter(":hidden").
:submit : for better performance in modern browsers, use input[type="submit"], button[type="submit"] instead.
:text : for better performance in modern browsers, use [type="text"] instead.
Remaining Tasks
The most recent patch was created several years before the switch to .es6.js files, so it's likely easier to recreate the changes manually vs applying the patch. An issue fork has been created with a few changes, and contributors should just expand on that as needed using the replacement pattern above.
Comment | File | Size | Author |
---|---|---|---|
#80 | 2153177-80.patch | 3.87 KB | Gauravvvv |
#79 | 2153177-nr-bot.txt | 150 bytes | needs-review-queue-bot |
Issue fork drupal-2153177
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
tarekdj CreditAttribution: tarekdj commentedComment #2
nod_Comment #3
nod_Good start, thanks!
The
:input
selector needs to be removed as well.Comment #4
nod_Comment #6
droplet CreditAttribution: droplet commented- can we don't drop the quotation mark ?
find('[type="file"]')
- add back input / button when it's possible ?
input[type="file"]
Comment #7
tarekdj CreditAttribution: tarekdj commentedNew patch supporting :input
Comment #8
nod_for :input it shouldn't be
find('*')
butfind('input,button,textarea,select')
,:input
should disapear from everywhere.Comment #9
tarekdj CreditAttribution: tarekdj commentedComment #10
tarekdj CreditAttribution: tarekdj commentedFixing accidentally removed line
Comment #11
nod_Looking at the sizzle source the replacements should be:
:radio, :checkbox, :file, :password, :image
replaced withinput[type=XXX]
:submit, :reset
replaced withinput[type=XXX], button[type=XXX]
:input
replaced with.filter('input,select,textarea,button')
Let's be explicit about the replacements and not just select things without tag name.
The content_translation file is borked, needs works.
The formEditor file should just send the string as the filter for the event delegation, no need to select all the elements. Also there is a bug, you're using a variable that you defined in another scope. Run JSHint before you send the patch :)
In tableresponsive, why do you select the parent() element for the headers? That shouldn't be needed.
Getting there :)
Comment #12
tarekdj CreditAttribution: tarekdj commentedI hope this time it will be good :)
Comment #13
nod_dropped the changes in form.js, it'll get rewritten in another issue. And the extra semicolon. While it's a valid fix it's out of scope.
The changes in tableresponsive will not work like that. Let's leave it for another issue.
See interdiff. You're removing pieces of a selector that should be needed, it'd be even better if you try to test the changes you make so you can catch syntax error and things that don't work properly.
Comment #14
nicobot CreditAttribution: nicobot commentedRe-rolled
Comment #15
nicobot CreditAttribution: nicobot commentedTested changes and following coding standards.
Comment #16
nod_you can't RTBC a patch you worked on :)
Comment #17
nicobot CreditAttribution: nicobot commentedThe re-rolled was quite simple and direct, but I'm fine with it :)
Comment #18
nod_True, it would have been fine if it was RTBC previously. The ways of the core queue :p
Comment #19
tarekdj CreditAttribution: tarekdj commentedComment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedAfter applying the patch from #14, a double space was introduced where there should only be one (content_translation.admin.js line 87, between click and table):
$('body').once('translation-entity-admin-bind').on('click', 'table .bundle-settings .translatable input', function (e) {
Comment #21
droplet CreditAttribution: droplet commentedadd space after comma: input, button, textarea, select
Please explain why it dropped the selectors.
space problem
space problem
input[type="checkbox"]. don't drop the quote marks.
Comment #22
droplet CreditAttribution: droplet commentedComment #23
nod_When re-rolling you should know that the indentation standard changed for JavaScript files.
Comment #24
babruix CreditAttribution: babruix commentedRe-rolled patch from #14.
Applied suggestions 1-5 from #21.
About why it dropped the selectors:
Seems it was decided in #13.
Comment #25
babruix CreditAttribution: babruix commented#24
Comment #26
babruix CreditAttribution: babruix commentedAdded spaces between selectors.
Comment #27
rteijeiro CreditAttribution: rteijeiro commentedPatch applies well and nothing seems to be broken. Should I test anything else or is it a RTBC?
Comment #28
droplet CreditAttribution: droplet commentedjQuery :input = input, button, textarea, select
any reason it dropped the "select" & "textarea" elements?
add space after comma, please :)
.first() -> eq(0)
Comment #29
babruix CreditAttribution: babruix commentedAbout 1) I think it was only 'input' there, because there are no other elements.
If you think we still need all these selectors - 'input, button, textarea, select' I have added in this patch.
Also 2) and 3) fixed.
Comment #30
babruix CreditAttribution: babruix commentedComment #33
estoyausenteComment #34
estoyausenteRerolled.
Comment #35
estoyausenteComment #36
estoyausenteComment #37
boedah CreditAttribution: boedah commentedComment #38
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled!
Comment #39
emma.mariaComment #40
emma.mariaRerolled patch
Comment #41
nod_Are you sure about that CSS class change? wouldn't want to break stuff :)
Shouldn't be possible to use .once like that. Needs to be replaced with
.once('translation-entity-admin-hide').each(function () {…
Let's not change that one, the selector is not working like you'd expect. Like that it listen to click on all buttons, textarea and select of the page.
Similar thing going on here, let's not change it yet.
Same here.
Just needs some spaces after
,
but fine otherwise.This one looks wrong, let's not change it.
Comment #42
babruix CreditAttribution: babruix commentedConvert type selectors, changes from comment #41
Comment #44
babruix CreditAttribution: babruix commentedApplied changes from comment #41.
Comment #45
tarekdj CreditAttribution: tarekdj commentedMinor change:
this.$input = this.$parent.find('input[type="checkbox"], input[type="radio"]');
Comment #46
babruix CreditAttribution: babruix commentedAdded minor change from comment #45.
Comment #47
tassilogroeper CreditAttribution: tassilogroeper commentedthe patch produces merge conflict in ajax.js - due to outdated patch.
FYI working on it at BarcelonaCon 2015 extended sprint.
Comment #48
tassilogroeper CreditAttribution: tassilogroeper commentedthe straight re-rolled patch.
merge conflict in ajax.js has been removed during development of the master, no need for to fix something in there.
Comment #49
tassilogroeper CreditAttribution: tassilogroeper commentedcleaned the code up. now it is a bit more readable.
Comment #50
hass CreditAttribution: hass commentedthere is a tab in the patch
Comment #51
tassilogroeper CreditAttribution: tassilogroeper commented@hass: thanks, I have missed that.
no interdiff, coz empty space replaced by empty space...
Comment #52
tassilogroeper CreditAttribution: tassilogroeper commentedComment #53
hass CreditAttribution: hass commentedThis looks like the same twice.
Comment #54
er.pushpinderrana CreditAttribution: er.pushpinderrana as a volunteer and at Publicis Sapient for Publicis Sapient commentedRemoved the redundant line as per #53.
Comment #55
tassilogroeper CreditAttribution: tassilogroeper commented@er.pushpinderrana sorry, had this duplicated line inside. But we need to remove the later, not the more explicit and readable one, please. Patch atteched
Comment #59
droplet CreditAttribution: droplet commentedThanks ALL for your hard-working! Let's do it when we have more JS test cases covered.
Comment #71
bnjmnmComment #73
xjmPromoting to major alongside the parent.
Comment #74
mradcliffeI added the Portland2022 tag. I also added the Needs issue summary update tag to clarify what the next steps are based on the recent issue fork from @bnjmnm several months ago.
Comment #76
bnjmnmComment #79
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #80
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedI have re-rolled the patch for 10.1.x please review
Comment #82
smustgrave CreditAttribution: smustgrave at Mobomo commentedCleaning up the tags.
Searched fore each of the patterns manually in core and appears reroll got them.
Downgrading to normal though as if this was major like to think it would of been addressed.