Last modified: 2014-07-03 22:36:46 UTC
Created attachment 13817 [details] patch for jquery.tipsy.js remove duplicate declaration of variables, and correct usage of title when it's a string
tipsy supposed to work with either a string or a function returning a string for "title". however, due to a small but, it doesn't work with a string: you can use "title" (in which case the original title will be used), but if you use anything else, instead of using it, tipsy will try to us an attribute whose name is that string, instead of using the string itself. patch attached. peace.
/small but/small bug/ /try to us/try to use/
Can you please submit patches to our git (gerrit) development ecocycle?
I think the behaviour you're seeing is expected: see the "Slightly Advanced Usage" section of [1]. The "title" property is meant to hold the name of the attribute whose value is used for the tooltip. If you want to use a custom string, use the "fallback" property, like Twinkle does [2]. [1] http://onehackoranother.com/projects/jquery/tipsy/ [2] https://github.com/azatoth/twinkle/blob/master/morebits.js#L592
(In reply to comment #4) > Can you please submit patches to our git (gerrit) development ecocycle? unfortunately no - i did not go through all the steps required to install the "ecocycle" as you call it - i got stuck at some step (can't remember exactly where) and abandoned it. i might go back to it one day, but i doubt it will be soon. (In reply to comment #5) > I think the behaviour you're seeing is expected: see the "Slightly Advanced > Usage" section of [1]. The "title" property is meant to hold the name of the > attribute whose value is used for the tooltip. If you want to use a custom > string, use the "fallback" property, like Twinkle does [2]. > > [1] http://onehackoranother.com/projects/jquery/tipsy/ > [2] https://github.com/azatoth/twinkle/blob/master/morebits.js#L592 the behavior might be "by design" that does not automatically preclude it from being a bug (designs can also contain bugs). it is convoluted and does not make any sense. for the very rare case where you want to take the hint from some other attribute of the element (i can't think of a single case where such bizarre feature makes sense), one can always set title: function(){return $(this).attr('foo'); } (btw - it would make more sense to use "data" or "prop" than "attr" - strictly speaking only valid html attributes should be used with "attr") otoh, the much more standard case is to feed the text when creating the tip. the current code uses an extra, completely superfluous field of "options" named "fallback" for that. making a very common case more difficult (or at least convoluted), in favor of some corner case which is not interesting (and can be easily implemented in the rare case someone will actually want it) is a bug, IMO. however, i can concede that this behavior, though illogical, should be preserved for backward compatibility. i can not concede that removing the the extra variable declarations shouldn't be cleaned: var title, $e = this.$element, o = this.options; fixTitle($e); var title, o = this.options; clearly, declaring "title" and "o" twice is just a sloppy bug - the line was copied and moved, but the original line was not removed. peace.
sorry for my previous long response. the short and simple one is: "fallback" just doesn't work. the twinkle example is nice, but as far as i could see, it points to dead code - there isn't a single callsite for this function, and once they'll create one, they'll have to fix this, because it doesn't do what the programmer thinks it does. the tipsy doc page does work (not really), but in order to do so, it creates an empty attribute named "original-title" on the element. this is completely borked. i could go into a long explanation why "fallback" doesn't work, but the simple fact is, it doesn't. either try it yourself or believe my - i tried it, and it doesn't (in order to make it work, you must create an empty attribute on the html element - this is completely perverted). summary: my patch fixes a real bug, and does not break any real backward compatibility. "fallback" never worked.
I am a developer of Twinkle and I assure you that our tooltip code does work! More to the point, patching MW's tipsy in this way would render it incompatible with other versions of tipsy, which is confusing. Hence I think it should not be done.
(In reply to comment #7) > there isn't a single callsite for this function, Lines 254, 276, 571 of morebits call this function.
(If you have any particular trouble setting up git-review or git, especially on Windows, then I'll be happy to help if you catch me on IRC or via e-mail. You can also try using http://tools.wmflabs.org/gerrit-patch-uploader/ to submit your patches via your MediaWiki.org account.)
(The patch in attachment 13817 [details] here seems to be encoded in UTF-16, which apparently Bugzilla has problems with and won't show the diff, and I wouldn't be surprised if other tools refused to accept it too.)
(In reply to comment #9) > (In reply to comment #7) > > there isn't a single callsite for this function, > > Lines 254, 276, 571 of morebits call this function. you are correct - i missed those in the search (my bad - i searched with a space). however, this only works if the original "title" is empty. if you try to attach a tipsy tip to a node with existing hint (i.e., a non-empty "title" attribute), fallback is not usable. my patch does not break this case. with current code, fallback only works if the node does not have a title, or if the node has some other empty attribute, and you pass to tipsy "title" with the name of this attribute. my patch will not break the first case: if the original "title" is empty, fallback will still work. with my patch, you could use "fallback" even when the original title is not empty, by passing title: '', fallback: yourstring to tipsy. with current code, you just can't use fallback in this case. (of course, with the patch, you can cut the detour and just pass title: yourtring ) of course, with my patch, you could bypass "fallback" altogether by passing the actual string as "title". the only backward-compatibility case my patch *does* break is the case where you want a tip with the value of some attribute: for instance, you might want to see the ID of every node matching some selector, you could do $(selector).tipsy({title: 'id'}); this is such an esoteric use, that it makes no sense to force everyone who wants to return a known string will be forced to use a function in order to save this esoteric case from the need to write title: function(){return $(this).attr('id'); } bottom line: 1) apologies for my long and winding comments 2) my patch will *not* break the twinkle use case, or any other case where the "title" attribute does not exist or is empty, which are the only cases where "fallback" works anyway. peace.
(In reply to comment #11) > (The patch in attachment 13817 [details] here seems to be encoded in UTF-16, > which > apparently Bugzilla has problems with and won't show the diff, and I wouldn't > be surprised if other tools refused to accept it too.) strange. i used git diff > tipsyPatch on a linux box to create it, with a "mediawiki-core" that i cloned from github. i do not think i explicitly changed the encoding of anything - neither the source file nor the patch. what is the encoding of the original file in the repo? peace.
All MediaWiki files should be encoded as UTF-8 or plain ASCII. This one seems to be properly ASCII on my machine.
kipod: Any chance to put this into Gerrit by patching against https://git.wikimedia.org/blob/mediawiki%2Fcore.git/HEAD/resources%2Fjquery.tipsy%2Fjquery.tipsy.js ? Could use http://tools.wmflabs.org/gerrit-patch-uploader/ for this...
ooops - an oldie... i'll see what can i do - prolly not before the coming weekend. there are other tipsy-related things i'm trying to push. if we decided to ignore upstream (as far as i can see, upstream is dead), i'd like to submit a couple more improvements: -- add an option to instruct tipsy to stay alive when overing over the tip itself, not just the triggering element (this is very important with "rich" tips, which contain clickable elements like links etc.) -- add an option to instruct tipsy to show the tip relative to the mouse, rather than the element (win WP this is important when the tip is attached to an internal link, because those can begin on one line and end on the next, which may cause the tip location to be surprising, thus violate the "least surprise" rule) peace.
