Accessibility: Removes hard-coded columns in home facet lists

[ckaz]

• adds CSS column solution.
• based on dev branch
  Reason: Lists of the same type or belonging under the same heading must be kept intact even if they are rendered as multicolumn on screen.

[ckaz]

Sorry, the rebase wouldn't push so I made this new based on dev.

[crhallberg]

So far so good. It looks like this PR is missing a change where the new variable is used. More branch/PR diff issues?

[demiankatz]

As @crhallberg notes, it looks like changes to search.scss are missing (along with the updates to compiled.css). Please let me know if I can be of any help in sorting this out!

[ckaz]

As @crhallberg notes, it looks like changes to search.scss are missing (along with the updates to compiled.css). Please let me know if I can be of any help in sorting this out!

Sorry, silly me. Of course! Sorry for wasting your time.
The search.scss had mostly code style changes applied. I forgot to turn this off. These should not impact functionality, but improve readability.

The key change is this:

`
.home-facet-list {
column-count: $home-facet-list-column-count; // defines number of cols, define this in variables.scss
list-style: none;
padding-left: 0;
padding-right: 1rem;

@media (max-width: $screen-lg - 1) {
column-count: $home-facet-list-column-count-md-sm-xs;
}
}`

Cheers!

[demiankatz]

Thanks, @ckaz. See below for some minor cosmetic issues.

A bigger issue -- this seems to be forcing two columns by default now, rather than using two columns as an overflow. With default settings on my screen, here are before and after shots:

Before:

After:

Is there a way we can prevent multiple columns from activating before they are needed? Or do you prefer it this way? (I think it looks busier and harder to read, personally).

[demiankatz]

For some reason, your style modifications have indented this line too far.

Suggested change

	// Bulk action checkboxes:
	// Bulk action checkboxes:

[ckaz]

you are absolutely right

[ckaz]

@demiankatz thanks for those eagle-eyed corrections!

[ckaz]

A bigger issue -- this seems to be forcing two columns by default now, rather than using two columns as an overflow. With default settings on my screen, here are before and after shots:

Is there a way we can prevent multiple columns from activating before they are needed? Or do you prefer it this way? (I think it looks busier and harder to read, personally).

Indeed, we can add a custom height (introduced as a variable), after which columns break automatically.

[demiankatz]

It looks the the merge of #4224 has introduced a conflict here. Do you mind resolving it? I'll test again once that's straightened out. (This is the disadvantage to all the style changes -- they greatly increase the likelihood of conflicts while the PR is in motion).

[ckaz]

It looks the the merge of #4224 has introduced a conflict here. Do you mind resolving it? I'll test again once that's straightened out. (This is the disadvantage to all the style changes -- they greatly increase the likelihood of conflicts while the PR is in motion).

Ok, sure. Just committed the changes. Hope that solves it.

[demiankatz]

@ckaz, thanks for the latest updates. A few thoughts:

1.) If you want to keep the style improvements, it might be helpful if you could just open a separate PR with only the style fixes. I assume that doing this wouldn't be much work. We could then merge that first and subsequently reconcile changes with this PR to make the history easier to read and understand.

2.) Having a hard-coded height for the columns seems a bit at odds with having a column size setting in the FacetList content block. We still use the column height setting to decide how many values to display, but now the column splitting is based on an independent value, so the results are no longer predictable/controllable. Do you think it might be possible, rather than hard-coding the height, to instead put the column size value as a data attribute on the container element, and use Javascript to set the height dynamically based on text measurements? I realize that this is not as easy as it sounds because long text may wrap, which makes the necessary height unpredictable... but maybe there's some algorithmic way to figure out a reasonable value, given the right inputs.

Anyway, just to demonstrate why this is a concern, here are before and after shots of what happens if I set the column size to "2":

Before:

After:

3.) If there's no reasonable way to make the new behavior equivalent to the old behavior, maybe we should consider deprecating or removing the column size option on the FacetList content block. It also occurs to me that the hard-coded special case for the callnumber-first column is ugly and would be better addressed through some kind of configuration. That's probably out of scope for this PR, but I just wanted to mention it while we're contemplating major changes here anyway.

What do you think? @crhallberg is on vacation right now, but he might also have some thoughts on this to share when he returns next week.

[ckaz]

1.) If you want to keep the style improvements, it might be helpful if you could just open a separate PR with only the style fixes. I assume that doing this wouldn't be much work. We could then merge that first and subsequently reconcile changes with this PR to make the history easier to read and understand.

Not necessarily. We have a style linter in place that makes sure the entire team writes SCSS to the same guidelines, so code is uniformly readable. Maybe something worth considering or discussing for VuFind in general.

[ckaz]

2.) Having a hard-coded height for the columns seems a bit at odds with having a column size setting in the FacetList content block. We still use the column height setting to decide how many values to display, but now the column splitting is based on an independent value, so the results are no longer predictable/controllable. Do you think it might be possible, rather than hard-coding the height, to instead put the column size value as a data attribute on the container element, and use Javascript to set the height dynamically based on text measurements? I realize that this is not as easy as it sounds because long text may wrap, which makes the necessary height unpredictable... but maybe there's some algorithmic way to figure out a reasonable value, given the right inputs.

I guess that would be the ultimate solution. I though it shouldn't be too hard to predict the FacetList entry (we have protected $columnSize = 10; in module/VuFind/src/VuFind/ContentBlock/FacetList.php) and the height set, because every library kind of knows their home page facets (at least roughly) enough to know what to expect.
However, you are absolutely right. We should avoid erratic behaviour.

Since I'm almost purely an interface person, I would soon be out of my depth trying to get the column size into this. In addition, I think this might open a box of additional problem.

[ckaz]

3.) If there's no reasonable way to make the new behavior equivalent to the old behavior, maybe we should consider deprecating or removing the column size option on the FacetList content block. It also occurs to me that the hard-coded special case for the callnumber-first column is ugly and would be better addressed through some kind of configuration. That's probably out of scope for this PR, but I just wanted to mention it while we're contemplating major changes here anyway.

Indeed that might be the best way forward, removing the column size option. That would leave everybody free to experiment.

[demiankatz]

Not necessarily. We have a style linter in place that makes sure the entire team writes SCSS to the same guidelines, so code is uniformly readable. Maybe something worth considering or discussing for VuFind in general.

I'm always in favor of consistency and good style; if there's a tool you're happy with, please feel free to open a PR if it's not too much work, and then we can discuss/adjust as needed.

[demiankatz]

Since I'm almost purely an interface person, I would soon be out of my depth trying to get the column size into this. In addition, I think this might open a box of additional problem.

I'm interested to hear what @crhallberg and @EreMaijala think about all this. Chris is on vacation right now, but I'll touch base with him about this when he returns.

[EreMaijala]

I'm not sure I like a multi-column display even if it makes the page shorter. To me the result seems harder to interpret, and won't work properly with hierarchical facets. That said, I don't mind it being possible (we don't even use home page facets in Finna), but I'd rather make it opt-in for cases where it makes sense. And I suppose some of the visual dfficulty could be alleviated with e.g. background color, vertical lines separating different facets or such.

[ckaz]

I'm not sure I like a multi-column display even if it makes the page shorter. To me the result seems harder to interpret, and won't work properly with hierarchical facets. That said, I don't mind it being possible (we don't even use home page facets in Finna), but I'd rather make it opt-in for cases where it makes sense. And I suppose some of the visual dfficulty could be alleviated with e.g. background color, vertical lines separating different facets or such.

Hi @EreMaijala , thanks for your reply. I agree with your notion about interpreting the columns.
However, I think we have a misunderstanding here. For accessibility for non-sighted persons who rely on screen readers, lists under the same heading must not be split structurally in the html. They may be reshaped visually but must remain one <ul> per list or the context might get lost for people relying on screen readers. That's the entire point. I'm very happy with the columns as such (though I'm not a fan) -- we just got told off by our accessibility testing company for splitting the lists.

[ckaz]

Not necessarily. We have a style linter in place that makes sure the entire team writes SCSS to the same guidelines, so code is uniformly readable. Maybe something worth considering or discussing for VuFind in general.

I'm always in favor of consistency and good style; if there's a tool you're happy with, please feel free to open a PR if it's not too much work, and then we can discuss/adjust as needed.

Cheers. It may turn into a bigger batch of things, with all the files to be revised in the end, but I'll look into that!

[demiankatz]

Hi @EreMaijala , thanks for your reply. I agree with your notion about interpreting the columns. However, I think we have a misunderstanding here. For accessibility for non-sighted persons who rely on screen readers, lists under the same heading must not be split structurally in the html. They may be reshaped visually but must remain one <ul> per list or the context might get lost for people relying on screen readers. That's the entire point. I'm very happy with the columns as such (though I'm not a fan) -- we just got told off by our accessibility testing company for splitting the lists.

I think @EreMaijala's comment is just pointing out that in addition to the structural change to clarify lists for screen readers, it may be worthwhile to add a small visual cue to help separate the list for sighted users, since things kind of blend together in the current design. Something like a faint border dividing the facet field sections (like a left border on everything except the first column) might do the job. That's arguably a separate issue, but I agree that it might improve the readability.

[EreMaijala]

@demiankatz The point I failed to make was that it requires some effort to do the multi-column layout, and I'm not sure if it's worth it. But I'm fine if people actually like it that way. :)

[EreMaijala]

This is sort of oddly named. In Bootstrap you'd typically have something like $home-facet-list-column-count-md which would have effect unless $home-facet-list-column-count-sm is defined and so on. Would it be too much trouble to actually support multiple breakpoints? Depending on the number of facets displayed two columns might make sense on md screen but be too much on sm.

[ckaz]

Would it be too much trouble to actually support multiple breakpoints? Depending on the number of facets displayed two columns might make sense on md screen but be too much on sm.

Oh, surely not. I'll look into that.

[demiankatz]

@demiankatz The point I failed to make was that it requires some effort to do the multi-column layout, and I'm not sure if it's worth it. But I'm fine if people actually like it that way. :)

The original purpose of the multi-column layout was to make it possible to display twice as many values for some facets as for others. Whether or not we need that is a separate question, but if we take away some kind of multi-column mechanism, we'll just end up with some lists being arbitrarily longer than others, which is definitely not desirable. The template is currently hard-coded to only show extra facets for call numbers, but that's something that should be made configurable if we plan to retain the functionality.

And I guess I'm also making a separate point, which is that I think separating the facet lists from each other with some visual marker might be valuable, regardless of whether or not they contain multiple columns of values (though it's definitely most important when there are multiple columns, because that makes things especially murky).

[EreMaijala]

@demiankatz Yes, the visual separation would be useful in any case, but doesn't need to happen here for me to happy. :)