Tooltip: on close and destroy only set title if empty or undefined. Fixed #8925 - Changes to title attribute are reverted on close #1146
Conversation
…ixed #8925 - Changes to title attribute are reverted on close Ticket #8925 states that changes to the title attribute while the tooltip is open are lost on tooltip close. In the close and destroy functions, the title attribute is always written if a value was stored in the element on open. It is possible the attribute has changed and restoring the initial value may overwrite the current value. If the value is empty or undefined as set in open, do not set the title attribute. This fix has the limitation that if the user removed the title attribute or set the value to an empty string the original title value would be restored on close and destroy. Test cases 3 and 4 demonstrate this limitation.
|
Thanks for your help so far, I think I've got my git config correct now. $ cat ~/.gitconfig
[user]
name = Daniel DeGroff
email = xxxxxx@gmail.comWhere the xxxxxx@gmail.com address matches the address when signing the Contributor License Agreement. |
| @@ -317,7 +318,8 @@ $.widget( "ui.tooltip", { | |||
| clearInterval( this.delayedShow ); | |||
|
|
|||
| // only set title if we had one before (see comment in _open()) | |||
| if ( target.data( "ui-tooltip-title" ) ) { | |||
| // if the title attribute has changed since open(), don't restore | |||
| if ( target.data( "ui-tooltip-title" ) && ( title === "" || title === undefined ) ) { | |||
There was a problem hiding this comment.
( title === "" || title === undefined ) could be replaced with !title, once more below.
There was a problem hiding this comment.
Good point, thanks. I replaced this with
!target.attr( "title" )
|
Apart from the mostly minor issues I've commented on above, I've ran the unit tests locally in Chrome 31 and through BrowserStack on IE 7, 8, 9, 10 and 11. Those caused plenty trouble with tooltip in the past, but everything is green. @robotdan can you fix the stuff I mentioned? Afterwards this should be good to land. Thanks. |
…ixed #8925 - Code review updates Code review changes.
|
@jzaefferer I've fixed everything you mentioned. Thanks! |
| @@ -317,7 +317,8 @@ $.widget( "ui.tooltip", { | |||
| clearInterval( this.delayedShow ); | |||
|
|
|||
| // only set title if we had one before (see comment in _open()) | |||
| if ( target.data( "ui-tooltip-title" ) ) { | |||
| // if the title attribute has changed since open(), don't restore | |||
There was a problem hiding this comment.
Comments should start with an uppercase character. Same below.
|
I noticed two more minor issues. The comments I'd just fix myself when merging, but would be good if you could address the other one. Would also like to get another reviewer to look at this, since these seemingly innocent changes tend to bite us later. @tjvantoll or @scottgonzalez, could you take a look? |
…ixed #8925 - Updates for code comments. Uppercase code comment, and refactor unit tests to allow for more variations without duplicate code.
|
@jzaefferer I fixed the uppercase in the code comment, and inverted the assertion to use equals instead. I also refactored the test case a little to make it easier in the future to add variations without duplicating code. |
|
Code looks good and it works. 👍 |
|
@jzaefferer In the future, don't forget to modify the commit message to reference the PR so that the discussion and original modifications can easily be found from the commit. |
|
Yeah, missed that. Thanks for the reminder. |
Tooltip: on close and destroy only set title if empty or undefined. Fixed #8925 - Changes to title attribute are reverted on close
Ticket #8925 states that changes to the title attribute while the tooltip is open are lost on tooltip close.
In the close and destroy functions, the title attribute is always written if a value was stored in the element on open. It is possible the attribute has changed and restoring the initial value may overwrite the current value.
If the value is empty or undefined as set in open, do not set the title attribute.
This fix has the limitation that if the user removed the title attribute or set the value to an empty string the original title value would be restored on close and destroy. Test cases 3 and 4 demonstrate this limitation.