nextcloud / photos

📸 Your memories under your control
GNU Affero General Public License v3.0
559 stars 61 forks source link

[BITV] 9.1.3.1b/4.2 - The "media" list was implemented semantically incorrectly - the "li" elements are not implemented as children of the "ul" element, but are inside "div" elements. (1) #1665

Closed AndyScherzinger closed 1 year ago

AndyScherzinger commented 1 year ago
8dbb463b3c7828316ae50cb029cebd52

0e4660212b80e42b753b1adcc08a8370

Details https://report.bitvtest.de/default-en/d63601ac-cb34-4645-8256-66bec78964a0.html#checkpoint-1d5eadb58d-v4-n2
JuliaKirschenheuter commented 1 year ago

Dear @michaelnissenbaum,

I've looked into current implementation and changed the structure to the semantically right list but that solution breaks our layout ;( I would like to ask you if it would be ok to create only container elements with <div> for photos and videos? Amazon photos and Google photos have implemented a media list this way:

3842-1882-max

Screenshot from 2023-05-03 16-15-05

Is there something more to pay attention on (probably some aria-label)?

michaelnissenbaum commented 1 year ago

Hi @JuliaKirschenheuter. The two examples you sent me, from my perspective, are not truly accessible. The use of lists could theoretically be avoided, but in order to do so, the headings for each photo would need to be defined. As I see it, example number 2 demonstrates this possibility. However, we should only use headings when content is being overridden (i.e. further content follows). In our case, this is not the situation, so the only option remaining is to use lists.

AndyScherzinger commented 1 year ago

@skjnldsv would you have an idea on how we could implement a mosaic view of photos (like we have) using a (ul-) list.

@michaelnissenbaum do you have any examples of a accessible implementation of such a gallery of images that follows the visual style of a mosaic-style?

JuliaKirschenheuter commented 1 year ago

@skjnldsv would you have an idea on how we could implement a mosaic view of photos (like we have) using a (ul-) list.

Please look into my current solution: https://github.com/nextcloud/photos/pull/1764

michaelnissenbaum commented 1 year ago

Hi @AndyScherzinger. I don't know whether this example is exactly what you are looking for but hopefully it can helps - https://www.smashingmagazine.com/2021/10/build-expandable-accessible-gallery/.

skjnldsv commented 1 year ago

@skjnldsv would you have an idea on how we could implement a mosaic view of photos (like we have) using a (ul-) list.

I've read the discussion on the associated PR, and to be fair, I'm not sure this is possible as it is. I think it might need some rewrite of the rendering algorithm to allow all children text to each others.

Also, that would rise another question, which semantic must the title have? Because they're all in the same rendering block (and they cannot be done differently with virtual scrolling), so a Date title would be a sibling of above/bellow photos. Not sure how to go around that to be honest.

JuliaKirschenheuter commented 1 year ago

Also, that would rise another question, which semantic must the title have?

@michaelnissenbaum seems to be that this is one way we could follow for now. Could you give us some info regarding this way of implementation? Thank you!

michaelnissenbaum commented 1 year ago

@JuliaKirschenheuter To finalize the solution, let's gather everyone together briefly to discuss all possible scenarios. Otherwise, there is a risk of talking past each other.

AndyScherzinger commented 1 year ago

@artonge can you post a code snippet of the list with some images / headlines in for @michaelnissenbaum to approve that this resolves the matter? (Since we currently don't have a demo system for the main branch or anything beyond v25). Thanks in advance.

artonge commented 1 year ago

Can the screenshot in the PR suffice ? Thinking about it, the month headers are also li elements, but that might be wrong.

AndyScherzinger commented 1 year ago

Can the screenshot in the PR suffice ?

No, given your follow-up question ;)

Thinking about it, the month headers are also li elements, but that might be wrong.

This is the question, because it might be okay (as in good enough) while it might be desirable to have a hierarchical list with 2 levels with the headlines on level one and a level2 ul for the images. That way the list would better reflect the grouping element the headlines represent. But I can't tell if or how easy this can be done because I expect that the server-provided data-structure doesn't allow for that. Which we could change for 28 then as a breaking change of course. So given the current screenshot I can't tell how the markup does look like exactly - for Michael to judge if this resolves the issue or needs further optimizations.