Datepicker: Get selectedDay from data-date instead of element contents

[c-lambert]

selectDay() use jQuery html() method on td cell to get the day value from DOM, but when user use translator or other extension, it return NaN value because the cell can contains non excepted content. We must use data- attribute to ensure that value cannot be altered.

• https://jqueryui.com/resources/demos/datepicker/localization.html
• https://translate.google.com/translate?hl=en&sl=fr&tl=pl&u=https%3A%2F%2Fjqueryui.com%2Fresources%2Fdemos%2Fdatepicker%2Flocalization.html

We got

<td class=" " data-handler="selectDay" data-event="click" data-month="0" data-year="2021">
    <a class="ui-state-default" href="#">
        <font style="vertical-align: inherit;"><font style="vertical-align: inherit;">22</font></font>
    </a>
</td>

instead of

<td class=" " data-handler="selectDay" data-event="click" data-month="0" data-year="2021">
   <a class="ui-state-default" href="#">22</a>
</td>

so this cannot work correctly

 inst.selectedDay = inst.currentDay = $( "a", td ).html();

I think it's not a good practice to get value from html DOM element

[mgol]

Thanks for the PR!

@fnagel Are you able to review this?

[fnagel]

Looks like a replace of .html() to .text() would work too but using a data attribute feels more safe and sane.

@mgol What do you think?

@c-lambert Would you mind squashing the CGL fix commit into the actual one?

[c-lambert]

Looks like a replace of .html() to .text() would work too but using a data attribute feels more safe and sane.

@mgol What do you think?

@c-lambert Would you mind squashing the CGL fix commit into the actual one?

Hello ! I think this is more safe too. data- attributes is indeed already used in the template for year and month data.

:1813

( unselectable ? "" : " data-handler='selectDay' data-event='click' data-month='" + printDate.getMonth() + "' data-year='" + printDate.getFullYear() + "'" ) + ">" + // actions

[mgol]

@fnagel Data attributes make sense to me considering they're also used for year/month/etc. data. However, looking at the code, Datepicker reads the attributes directly via native getAttribute instead of using .data() which just pulls initial attribute value and then ignores subsequent changes to the attribute.

@c-lambert Such a change requires adding a unit test; you can look at the existing test files for similar data-*-related tests.

[fnagel]

Please change the commit title from "DatePicker: Get..." ti "Datepicker: Get...". Otherwise good to be merged IMO.

@mgol I think we can skip the tests here, as its actually a tiny change which should to be covered my existing tests.

[mgol]

@fnagel We don't have tests ensuring we don't read the element contents but I guess we also don't have such tests for other fields so I guess we're good here. I wouldn't block the RP over the title, I can fix that during the squash & merge. If that's your only concern, could you withdraw the objection? I'll be sure to modify the component during the merge.

[mgol]

Withdrawing my objections per Felix's comment; LGTM.

[mgol]

I looked at the PR again & I have one important remark.

[mgol]

Actually, now that I look at it again, we should read the attribute directly instead of using data. .data(key) pulls the initial value from the data-key attribute but that's it; it then keeps its state and disregards the attribute. It sounds we want the attribute to be the soruce of truth here.

Most other similar places in this widget use +this.getAttribute( "data-something" ). I'm not sure if skipping jQuery's .attr() had any reason but we should either use getAttribute or attr here & cast the value to number.

[mgol]

@c-lambert Hey, do you want to try to update the PR with my feedback? If you don't have time, we may try to do it ourselves.

[c-lambert]

@c-lambert Hey, do you want to try to update the PR with my feedback? If you don't have time, we may try to do it ourselves.

Ok done ! I also parsed the return value of attr() method to int.

[mgol]

@c-lambert Thanks! Can you also rebase over latest main? The PR cannot be merged right now due to a conflict.

[mgol]

@c-lambert I see you upvoted my comment. Let me know when you rebase and I'll merge the PR. Thanks!

[c-lambert]

@c-lambert I see you upvoted my comment. Let me know when you rebase and I'll merge the PR. Thanks!

I can do it this weekend !

[c-lambert]

@c-lambert I see you upvoted my comment. Let me know when you rebase and I'll merge the PR. Thanks!

Hello it's done !

[mgol]

Landed, thanks!