jquery.placeholder should take the placeholder value as first parameter
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Rillke
	Sep 21 2012, 4:11 PM

Description

7badb11ae872270d5b24f815617c772cc8287d7d

Usually, you create a new node with jQuery using the following syntax:
$('<elem>', map_of_methods_and_attributes);

Methods of $.prototype take preference over attributes. So, when using
$('<elem>', { paceholder: "text to be shown when empty" });
and the jQuery placeholder plugIn is loaded, it is called instead of setting the attribute.

Test case:
mw.loader.using('jquery.placeholder', function() {

// Prove that it is loaded
console.log('placeholder> ', mw.loader.getState( 'jquery.placeholder' ));
// Prove that the placeholder is not set
$('<input>', { 
    type: 'text', 
    placeholder: 'Hello!' 
})
.prependTo('#content');

});
Result: Empty text box without placeholder.
Expected: Placeholder text "Hello!"

This can be easily circumvented if $.fn.placeholder would take one argument:

-$.fn.placeholder = function () {
+$.fn.placeholder = function (text) {

and using this argument later
+placeholder = text || this.getAttribute( 'placeholder' );
+-if ( this.placeholder && 'placeholder' in document.createElement( this.tagName ) ) {
+this.setAttribute( 'placeholder', placeholder );
+- return;

-placeholder = this.getAttribute( 'placeholder' );

Version: 1.21.x
Severity: normal

Details

Reference: bz40430

Event Timeline

• bzimport raised the priority of this task from to Low.Nov 22 2014, 1:05 AM

• bzimport added projects: MW-1.22.0-release, MediaWiki-JavaScript.

• bzimport set Reference to bz40430.

Rillke created this task.Sep 21 2012, 4:11 PM

We could "fix" it in jquery.placeholder, but on the other hand it could also be used to make an example of: Don't use this "bad part" of jQuery.

Besides, we can't control every single plugin, there are also third-party plugins, plugins made on individual wikis and gadgets etc. Its a lost cause / useless battle.

What I mean is, even if we fix it in jquery.placeholder, it does not make it "right" to use this jQuery feature, because there can still be any other jQuery plugin active in your context that creates another plugin in that name, not to mention, it could even overload the jquery.placeholder plugin, so even that wouldn't be safe.

Given that it doesn't fix anything and only further encourages a bad practice, I'm inclined to say, wontfix.

To set attributes, use .attr() with key value pairs.

To call other methods, just call those methods. The syntax in jQuery has already been made a lot easier to work with. Now .attr(), .on(), .prop() and .css() basically handle 80% of everything with other methods only pointing to those. So if you like to use objects instead of calling a lot of methods, you can. Just have to call one of those 3 first, like:

$('<input type="checkbox">').attr({

foo: 'bar',
baz: 'quux'

}).prop({

value: 'Hello!',
selected: true

}).on({

click: function () {},
focus: function () {},
blur: function () {}

}).css({

...

}).data({

...

});

That's still pretty neat, but ensures that everything is very much explicit without "guessing" whether that key-value pair is an attribute, property or event.

Even when not considering plugins, the map in jQuery() constructor is still unstable. Because what if you want to set the "checked" property?

$('<input type="checkbox">', { checked: true }).prop('checked');

That will fail and return false.

(In reply to comment #0)

7badb11ae872270d5b24f815617c772cc8287d7d

What does this commit hash refer to? It doesn't appear to exist in the MediaWiki repository.

Usually, you create a new node with jQuery using the following syntax:
$('<elem>', map_of_methods_and_attributes);

Then I'd say that is a bad habbit

Test case:

// Prove that the placeholder is not set $('<input>', { type: 'text',

For you information, the "type" attribute should not be modified from javascript, because in IE input elements need to be given a type time of parsing, altering (even if directly afterwards) does not work, so this would have to be $('<input type="..">') instead. In the above case it works fine, but that's because type="text" is the default value, not because it "set".

(In reply to comment #1)

Given that it doesn't fix anything and only further encourages a bad practice,
I'm inclined to say, wontfix.

Ok. Adding explicitly as "discouraged" to Manual:Coding_conventions/JavaScript or just as a pitfall?
https://www.mediawiki.org/w/index.php?title=Manual%3ACoding_conventions%2FJavaScript&diff=586304&oldid=584823

(In reply to comment #2)

What does this commit hash refer to? It doesn't appear to exist in the
MediaWiki repository.

It was what I found below the heading on
https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/core.git;a=blob;f=resources/jquery/jquery.placeholder.js
I thought it would link back to this file... Well I have to get used with Gerrit.

For you information

Yes, I know IE 6 and 7. But I decided that I wouldn’t pay a lot of attention to them any more. People and enterprises can get excellent browsers for free.

This case is, however "special". It might be more intuitive if you could write
$elem.placeholder(text);
without having to set a placeholder before. (Just my opinion)

Change 74333 had a related patch set uploaded by Rillke:
jquery.placeholder: Take placeholder text as parameter

https://gerrit.wikimedia.org/r/74333

Change 74333 merged by jenkins-bot:
jquery.placeholder: Take placeholder text as parameter

https://gerrit.wikimedia.org/r/74333

Merged the change now, as using the plugin directly without needing to set the attribute sounds pretty neat and should be sane (in this specific case).

jquery.placeholder should take the placeholder value as first parameterClosed, ResolvedPublicActions

Description

Details

Event Timeline

jquery.placeholder should take the placeholder value as first parameter
Closed, ResolvedPublic
Actions