-
Notifications
You must be signed in to change notification settings - Fork 922
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
pkp/pkp-lib#7272 Simultaneously Displaying Multilingual Metadata on the Article Landing Page #4050
base: main
Are you sure you want to change the base?
Conversation
Hey @asmecher, I have PRs ready for review of the code... OJS: #4050 |
Thanks, @jyhein! I'd like to start by passing this by @Devika008; can you add a couple of screenshots that show the changes? |
Hey @Devika008 and @asmecher, Here are some screenshots: |
31520dd
to
f37b2a0
Compare
I'm sorry for getting onto this late. I might have missed it. However, I believe that when users read through the data, they wont read data in a way that article title in french in Spanish in Portuguese and then move onto the next one. They will go through the metadata together in a language. Sometimes its easier to scout for metadata when you know the language you're looking for. Hence I propose the following reorder of data with respect to clubbing the metadata in one language togehter. Its easier to process and find |
5d41251
to
a7ddb59
Compare
pages/article/ArticleHandler.php
Outdated
->concat(['keywords', 'abstract'])->unique()->diff($titles->keys())->values(); | ||
$multilingualOpts = collect($showMultilingualMetadataOpts) | ||
->when(in_array('title', $showMultilingualMetadataOpts), fn ($m) => $m->concat(['subtitle', 'fullTitle'])->unique()->values()); | ||
$primaryLocale = isset($titles->get('title')[$contextPrimaryLocale]) ? $contextPrimaryLocale : $publication->getData('locale'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not correct: why do we use the context primary locale?
See how the function DataObject::getLocalePrecedence() works (https://github.com/pkp/pkp-lib/blob/main/classes/core/DataObject.php#L97), used in DataObject::getLocalizedData() (https://github.com/pkp/pkp-lib/blob/main/classes/core/DataObject.php#L65):
First either preferred locale will be considered, if given (e.g. if you want to get a key/attribute in a specific locale).
If preferred locale is null, the UI locale is considered.
If there is no value for this locale, then the fallback locale (getDefaultLocale()) is considered. For Publication object it is the submission primary locale (s. e.g. PKPPublication::getDefaultLocale()). This value should actually always exist for required fields.
And if no value is found here, e.g. for keywords that do not necessarily need to be in the submission primary locale, then check context and then site primary locale.
If no value exists for any of these locale the function getLacalizedData() will return the first found value.
Thus, I think we should do the same here, no? Also, if possible to use that getLacalizedData() function.
Also, on the article view page:
With this implementation the context primary locale instead of UI locale (that exists) is displayed. For example, let say context primary locale is FR, and the article primary locale is EN, and metadata exist in both languages (EN and FR). When the UI locale is EN, the article is displayed in FR, although EN exist.
If the UI would be DE, and article does not have metadata in DE, the EN should be displayed, because EN is the submission primary locale.
I do not know if Devika said something about the situation when e.d. keywords only exist also in DE, additionally to the example above.
Currently it is so: if the UI = DE, then title and abstract are displayed in EN (because they do not exist in DE, but in submission primary locale), but keywords are displayed in DE.
I think for now we should keep this, if Devika has said nothing about it now, how it woks now. And I think Devika wanted to think further about how to improve it all...
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed context->getPrimaryLocale() to templateMgr->getTemplateVars('currentLocale'). If no "show multilingual metadata" options selected in the default theme settings and metadata does not exist in title's locale, then the metadata is shown in some other language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jyhein, I have not taken a look into the code yet, but I tested it, and there is still some difference from how it was till now:
Lets say the journal has EN, FR, DE, and SP enabled as UI and submission languages.
Lets say the article primary locale is EN, and it has all metadata also in FR, and additionally it also has author metadata and keywords in SP.
When the reader select SP as UI language:
Now it is so:
The article title and abstract are displayed in EN, author metadata in SP, and the keywords also in EN, although the keywords in SP exist. Then the article metadata in SP are considered next, and there the keywords in SP are displayed.
If the new option to show metadata in all languages is not selected in Website Settings, then the SP keywords are never displayed.
Thus, I think it should be:
The article title and abstract are displayed in EN, author metadata and keywords in SP. Then the other languages are considered the data exists for, i.e. EN and FR.
Thus, could we do something like this:
For the first/main display of metadata use getLocalizedData as till now. This first/main display considers the selected UI language. It will return either the data in the UI language or the data in article primary language or any other existing language.
Then for the new functionality, to display metadata in further languages: Get all existing article languages for the selected metadata from the settings (title, abstract, keywords), remove the UI language from that list (because it is handled as first), then for each language from that list display existing metadata using getLocalizedData(key, language).
I am not 100% sure, but maybe in the handler class you would 'only' need to get those further languages that needs to be displayed (= get all existing article languages for the selected metadata from the settings (title, abstract, keywords), remove the UI language from that list). And then in the template this can be done: for each language from that list use getLocalizedData(key, language) to display metadata in additional languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bozana! Default view (no metadata options selected): the code tries to give the metadata based on the title language (if not ui then submission). If the metadata does not exist in title language, then the getLocalized data selects some found language. I tried to avoid to display a mix of languages as much as possible. I can change this to match the original logic, or I could display the other languages the same way like in the multilingual view.
Multilingual view (one or more options selected) : Using getLocalizedData won't work here because if in UI/submission language e.g. keywords do not exist, the function would show it in other language. In this view it would be inconsistent.
I have not included author metadata in to this but I can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jyhein, I am hesitant to change how it worked till now -- it was so for a long time... :-) Also because maybe not all users will use this additional function and the system should behave as always. The keywords in SP from the example above should be visible. (Also, users could extend the default theme to also display other metadata (e.g. coverage, sponsoring agencies, etc.) and they would probably follow the same logic, and they all should be visible in default view).
And because Devika will maybe come with a fully different model/logic later... So lets keep this as it was, for now, i.e. with minimal changes, if possible.
Thus, I think default view (no metadata options selected) should work as till now.
The multilingual view (one or more options selected): the first metadata display should stay as it is, as in the default view, and only the other existing languages (different than selected UI language) should be added.
Sorry, I was wrong about getLocalizedData for additional (other than selected UI) languages: it should be getData(key, locale)....
You do not need to include author metadata -- it was just an example, that they can also be in other languages, i.e. in SP in the example above and would be visible...
f95676a
to
0baa986
Compare
Hi @Devika008, Article Metadata in EN Article Metadata in FR That means under "Article Metadata in EN" the title and abstract will be shown, even they are also shown in the first element (for UI = SP) because they do not exist in SP. Else, @jyhein, tried not to mix the languages, so that way it now would be: Article Metadata in SP Article Metadata in FR This looks maybe OK when the option to show the metadata in multiple languages is selected, but when that option is not selected, the SP metadata will never be displayed, which I think is not a solution. Also, this changes the way how it was till now. Thus, just to double check with you what do you think is better for now, and to let you know about this edge case when you work on the default theme further. Thanks a lot! EDIT: I think we could also consider authors metadata in the #2, so that not only they are displayed in UI=SP in the first element, and we could maybe differentiate the situation when the option to show the metadata in multiple languages is selected or not (so that the SP data are shown if that option is not selected), but it is all even a bigger complication. |
@Devika008, maybe just a note: I have just spoken with @jyhein and we will then try to have separate solutions for those that have the multilingual view enabled and those that do not have it (which will work as till now). @jyhein will document it in the issue on Monday... I still think that we are maybe investing too much time now, because we will need to rethink it all soon (when submission and metadata languages are separated from the UI languages), and eventually when UI language will be accessible via URL... but... |
4b735dc
to
aed56ac
Compare
Hey, @Devika008 Here are some some cases when metadata exist partially only:
Case 1: Only keywords in Swedish:
Case 2: Only title in Swedish
|
@bozana I think your examples show that the real problem we have in OJS (and which is underlined now more) is the concept of mixing metadata based on the UI language. I mean this:
This is confusing for the reader especially in cases where the two languages are close to eachother.
I do not think that having the UI language accessible via URL will affect this. It will just change the way language switching is happening. The real question is that should the UI language affect submission metadata view especially in cases where the full metadata is not available for the selected language. In my opinion such functionality should only happen when the full text is multilingual and in all other cases we should always respect the Submission Locale. But I think this issue is separate from that. The real use case for this is a situation where only small parts of metadata (like the abstract) is available in other language and the editors want to show that on the same landing page. IMO we should not try to extend this further from that concept. |
@ajnyga, yes... So what do you think is best for this issue, now? EDIT: because according to the design, we do not display the abstracts one after another (which would be much easier), but first all metadata in one language and then in other, and then other,... (for all existing metadata languages)... EDIT EDIT: I am not thinking about the way we will solve the UI, submission and metadata languages and URL languages now -- we will wait for Devika's concept... |
Hi all, I believe this could be maybe the next issue we can finish :-)
If the journal has additionally FR as submission and metadata locale, and FI as metadata locale. How would those be considered/displayed, if the article has all data in FR and some metadata in FI? Thanks a lot! |
Just want to underline two things:
|
65fd6ac
to
7d7cde1
Compare
Hey folks, I'm just getting caught up on this conversation and trying to wrap my head around it. I think any movement in this direction is good. Off the top. I just want to say that! Devika, I appreciate all the work you've put into these. I think there's more than one reason for journals to try to display multilingual metadata. Marc said this:
I think this is only half true. The other objective is to give journal options for displaying multilingual metadata so that they're not tempted to jam all their metadata into one field to get them to display at the same time. When I see errors in this space it is almost always about display. "I want to see both". "I want our title to have both languages". "I want both abstracts to be viewable on one record without having to search for it." But, it's possible that this is a cultural difference between what I've seen with Canadian journals and what I've seen with other cultures where bilingualism is a lot less... uhhh... federally complex. But I also want to raise something that A-J said further down:
This is where I think we start to run into problems with multilingual metadata for real. Downstream. When the metadata leaves OJS for other systems. There's two problems:
And Devika has indicated this. And I think the side-by-side option for displayed stuff works. But I do think we need to figure out what we mean by "completeness" in the multilingual space before we figure out what "completeness" in the UI/UX space is. Basically, I don't really know if users need an at-a-glance multilingual view for, like, affiliations. They need titles, abstracts, and then galley labels. We know that much. And I think for major UI/UX refactors knowing what metadata users need, specifically, in this kind of view is good. I'm not confident it needs to be "all available metadata". And, actually, I almost think we should do something more like a DSpace or repository record where you can view all item metadata external to the article view UI. I think I'll also tag @mtub here. I do think the conversations around what's displayed for multilingual metadata and how users see it is a huge issue. There's such a balance between:
|
5a8eef9
to
2c87b0c
Compare
a35d0e6
to
6c3ab81
Compare
Hi, just let you know, I have been reviewing the metadata switching on article page. But still have not figure out how to proceed exactly with it yet. Main issue I have there is with that metadata switcher. From seeing the interactive mockup - it works exactly as html select element - just with custom look. You click on it, and than you can select other one and once you select other one it closes. So I think best for accessibility would be to code it like that. Example of such component with correct accessibility is for example here - https://headlessui.com/v1/vue/listbox (when using for example such headless component its quite straightforward to have custom look as in mock up) And ideally organise it is so the code of the select component itself is not entangled as much with the metadata changing use case. On the reader side we have limited options (no vue.js yet). I have been looking to alpinej, which currently offers headless components and is good fit - does not require build step and is good for adding some JS interactivity. I managed to make it work with it - avoiding any custom JS and relying on alpine for accessibility. But realised that the problem might be that even all alpine code is MIT - the documentation for components/headless components are paywalled, so I am not sure how much we would want to encourage using it going forward. So need to check with team and explore if I can come up with some other alternative. Basically to avoid re-implementing listbox correctly. Ideas/thoughts welcomed. |
@jyhein Here is example how the alpine select can be used, so we don't have to reinvent accessible select - 4bf3847 Here are suggestions to improve things
|
Thanks for the suggestions, @jardakotesovec I have few comments:
|
@jyhein Hi Jyrki, Have you done some screen reader testing? Multiple suggestions is really motivated by testing voice over in safari. And thinking how to semantically encode this. I see there are some use of aria-expanded, but from testing did not seem that functional. So first thing I was really trying come up with is how to encode the language switcher. And when checking Devikas interactive mock up I came to conclusion that it behaves same as html select component, just with different styling. Benefit is that with screen reader you will hear that there are multiple options (and how many) and that you are moving between them with clear understanding that you are selecting one of them. Having in some cases the locale switcher inside h element and sometime outside is also concern about having good semantic html so its well interpreted by SR. Your option when it closes on the outside click is nice - but not sure how to code that. If you come up with some SR friendly way I am not against it. Other motivations are really just to keep the template and css changes minimal so its very easy to understand and customise as the default theme is the go to theme for making customisations and child themes. Regarding animations I am somewhat indifferent - thing is that for multiline titles part of the page is jumpy anyway.. So my motivation here is just to keep things simple, but thats something we can get back to and also get Devika preference. |
Screen reading part I can improve. I can move all the switchers outside of h-elements, it does not matter where they are in the code. Most of the template changes come from the foreach loops to add the multilingual texts, to add the switchers is just adding data-pkp-* -attributes and the element where to target the switcher. It is easy to understand from the examples. And one can choose to add just one switcher to change all the texts on the page. |
@jyhein Oki, sounds good, looking forward to the update. Thanks. |
@jyhein wondering whether passing the other languages as json object to the page could help with simplifying the template part. So it would not need to iterate over languages - but instead it take it from JS object and replace the text inline. What you think? |
OK, sounds good |
Hello all, I've been testing this feature on a sandbox OJS installation. It is a nice option to be able to switch the language of specific components to improve the accessibility of the content to users who can read in other languages. The built-in language switcher is pretty handy. Focusing on the switcher only, this is the parsed HTML on the page:
I've attempt to switch the language using keyboard navigation only, and it was possible, not easy though. Also, the HTML is semantically valid. However, there is a critical lack of accessibility that I noticed while testing with 2 screen readers (SR)(VoiceOver on MacOS and JAWS on Windows):
Possible remediation to fix the issues above should include:
I will paste bellow a possible HTML remediation ( I changed the HTML only, the CSS will require further adjusting):
Key improvements made:
This chunk can be simplified if we use |
@israelcefrin Thanks Israel for the notes. I also did some testing with voice over on mac and it does not behave as listbox (it does not announce the options) It can be compared with some existing implementation like - https://react-spectrum.adobe.com/react-aria/Select.html I suspect that the main reason is that once is clicked on the 'en' button - it will change the whole thing to be listbox. So there is no connection between having button thats opening listbox. And thats what I suspect confuses screen reader. Its not trivial to get these things right.. thats why I was suggesting to use headless dependency for this, even though having additional dependency is also not great. But I still think it might be better choice than trying to reimplement listbox correctly. |
Hello @jardakotesovec
That's is likely true. ARIA is a great way to implement accessibility features, but it may be tricky sometimes. Also, I've painfully learned that with OS / Browsers / Screen Readers, you always need to close all browser windows, open screen reader, and re-open the testing browser. For some reason, SR may have an erratic behaviour to connect and read the DOM tree from already open browsers. At the end of the day, that's how actual SR users experience the interaction too. If you run in some non-expected behaviour when testing a component/ARIA, try to close both SR and the browser, and re-test launching the SR first. In Linux this protocol is even documented since they recommend to launch ORCA before Firexof to navigate.
What is headless dependency for this component? I am not familiar with the term. (and google didn't help much) |
Hey @israelcefrin, |
@israelcefrin In react/vue ecosystem there is lots of headless libraries, that will handle accessibility and interactivity, while give you full power of custom styling. Outside of these ecosystems I came across alpinejs headless components https://alpinejs.dev/components , that are realtively easy to integrate with just html. Unfortunate is that the docs is behind paywall. But also understand that they need to support project somehow. Actual code is opensource. I tried to apply it in our case - its covered in this comment - #4050 (comment) . Biggest pain point is that paywall. But in general these libraries are great if done well.. to avoid reinventing these patterns that have clear specification. |
Hello @jyhein , I am running tests using ScreenReaders to emulate the user experience. There is one issue that I can't find the cause. After I change the Heading level 1 language, JAWS screenreader announces it 3 times. It happens only when I change the language. If I navigate the page, the component is announced only once (as it should be). |
Hello @jyhein , just confirming that removing the |
Hello @jyhein , I've noticed that you've added tabindex="0" and tabindex="-1" to the elements of switching language components. The latter removes them from ScreenReader reach (DOM tree).
Is there any specific reason to add the attribute? I run a final test using keyboard only navigation + Windows SR (JAWS and NVDA) and I am not able to switch the languages. VoiceOVer in MacOS doesn't seem to bother though. |
Issue: pkp/pkp-lib#9373