Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datepicker: removed arrow symbols from next and previous month hovertext in swedish #2050

Closed

Conversation

adalid-1
Copy link
Contributor

Fixes #2048

@linux-foundation-easycla
Copy link

CLA Not Signed

1 similar comment
@linux-foundation-easycla
Copy link

CLA Not Signed

@adalid-1
Copy link
Contributor Author

Commit is made from work git account and PR is made with private git account.

@mgol
Copy link
Member

mgol commented Feb 11, 2022

@adalid-1 can you amend your commit so that the commit author info matches an account with which you can sign a CLA? A CLA signature is a hard requirement for us to accept contributions.

@mgol
Copy link
Member

mgol commented Feb 11, 2022

@fnagel any idea why these symbols were included in some language versions, leading to issues like #2048, and not in others, like en-GB?

@adalid-1 adalid-1 force-pushed the Remove_Arrows_From_Datepicker_tooltip branch from 2baed25 to cefc4dc Compare February 11, 2022 15:36
@adalid-1
Copy link
Contributor Author

@mgol I rolled back the local branch and recomitted the change with correct author and did a forced push. I am not great with git so please be wary if you see something wierd. If so I can remake the pullrequest from scratch after the weekend.

@fnagel
Copy link
Member

fnagel commented Feb 11, 2022

Afaics we have the following prefixes for prevText and nextText:

  • < &#xAB;
  • « &#xab;
  • < &#x3C;
  • &larr;
  • ← and → actual UTF symbols

@mgol

any idea why these symbols were included in some language versions, leading to issues like #2048, and not in others, like en-GB?

No, not really. And there is no consistency. Which is odd. Why would we have those in some languages only. Makes no sense to me. So either remove or at least using the same char.

Looks like the majority is with one of those prefixes, but according to #2048 those chars could cause issues so removal might be the better option. I don't think its a good thing for a11y.

What do you guys think?

@mgol
Copy link
Member

mgol commented Feb 14, 2022

Yeah, I'd also opt into removing all of those prefixes/suffixes. @adalid-1 would you like to do it for all the locales?

@adalid-1
Copy link
Contributor Author

@mgol Sure thing!

Removed arrow symbols for next/previous month in all languages

Ref jquery#2048
@adalid-1
Copy link
Contributor Author

A few languages turned up empty when removing arrow. Maybe that is ok?

Removed in some languages i missed first time

Ref jquery#2048
@adalid-1
Copy link
Contributor Author

Do I need to do anything more here or can I just wait for someone to review and merge this?

@mgol
Copy link
Member

mgol commented Feb 21, 2022

Hmm, I wonder if this is really behaving in the same way as 1.12.1 did. As prevText & similar were documented to accept text, not HTML but were in fact accepting HTML, we switched that to only accept text as a security fix. It looks like this is something that broke those arrows... @adalid-1 are you sure the behavior in 1.12.1 was the same?

Making the texts empty may be bad for accessibility, potentially worse than at least having an arrow (as long as it works, of course). Maybe we should leave the arrows - but fixed, inserted as Unicode, not HTML entities - in those languages that would otherwise have empty prevText, etc.? What do you think, @fnagel?

@adalid-1
Copy link
Contributor Author

You are right it works in the older version. I was misstaken, I must have made some changes that did not come through when testing earlier. Now that i tested downloading the older version from here:
https://jqueryui.com/download/
And setting nextText to contain the arrow it came out as intended.

@fnagel
Copy link
Member

fnagel commented Feb 23, 2022

As prevText & similar were documented to accept text, not HTML but were in fact accepting HTML, we switched that to only accept text as a security fix.

Sounds reasonable. Anyway, a fix would be good.

A few languages turned up empty when removing arrow. Maybe that is ok?
Making the texts empty may be bad for accessibility, potentially worse than at least having an arrow (as long as it works, of course).

We should definitely not have those texts empty. I'm ok with using the arrows but having a real text would be better. I could add a commit introducing those missing texts translated by Google Translate (and verified by another). @mgol What do you think?

@mgol
Copy link
Member

mgol commented Mar 1, 2022

I could add a commit introducing those missing texts translated by Google Translate (and verified by another). @mgol What do you think?

@fnagel If you could do that, that'd be great! We just won't be able to verify those easily and I had an impression you prefer to be more cautious here. Please let me know when you do it so that we can proceed with the rest of the changes.

@fnagel
Copy link
Member

fnagel commented Jul 8, 2022

We might want to merge #2067 before working on this. This one seems related: #1900

@fnagel fnagel self-assigned this Jul 8, 2022
fnagel pushed a commit to fnagel/jquery-ui that referenced this pull request Jul 9, 2022
fnagel added a commit to fnagel/jquery-ui that referenced this pull request Jul 9, 2022
fnagel added a commit to fnagel/jquery-ui that referenced this pull request Jul 9, 2022
mgol pushed a commit to fnagel/jquery-ui that referenced this pull request Jul 14, 2022
mgol pushed a commit to fnagel/jquery-ui that referenced this pull request Jul 14, 2022
mgol pushed a commit to fnagel/jquery-ui that referenced this pull request Jul 14, 2022
@mgol mgol closed this in 3126e12 Jul 14, 2022
mgol pushed a commit that referenced this pull request Jul 14, 2022
mgol pushed a commit that referenced this pull request Jul 14, 2022
@mgol
Copy link
Member

mgol commented Jul 14, 2022

This landed in 3126e12 as part of #2100; your author info has been preserved. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Datepicker, Next month button tooltip encoding issue?
3 participants