Problem/Motivation

- We have a design for the breadcrumbs component and now it's time to implement this feature in the Olivero Drupal theme.
- Link to the design - https://www.figma.com/file/r8uq72Q9weTMVsNUFZKEYH/D9-Theme-Master?node-i...

Olivero Breadcrumbs

Proposed resolution

- We're using the default breadcrumbs block for the theme.
- Configure the block and export the `.yml` config into this directory `/olivero/config/install/`.
- Theme/style the breadcrumbs to match with the design for desktop and mobile breakpoints.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

proeung created an issue. See original summary.

proeung’s picture

Issue tags: +olivero-sprint-d
thejimbirch’s picture

Status: Active » Needs review
FileSize
694 bytes

The default breadcrumb block config is already in the repository at https://git.drupalcode.org/project/olivero/blob/8.x-1.x-dev/config/insta...

The current CSS in the theme from the Proof of Concept (POC) does not align with the designs. What is in the Figma link, and screenshot above is bold, not uppercase, etc.

Attached is a patch that updates the styles in the theme to match the designs, and adds an ::after to override what I believe is Drupal's default behavior.

Ujval Shah’s picture

@thejimbirch I have tested the patch. Please find below findings:

  1. Separator looks to be different, used code in the before tag outputs different separator compared to the design.
  2. Space between separator is also different.

Other Findings:

  1. White space is applied for breadcrumb items. Space can be managed using the margins around separator.
proeung’s picture

Issue summary: View changes
proeung’s picture

Issue summary: View changes
thejimbirch’s picture

Status: Needs review » Needs work
mherchel’s picture

Issue summary: View changes
  1. +++ b/css/src/components/breadcrumb.css
    @@ -7,10 +7,12 @@
    +  padding-bottom: 20px;
    

    use var(--sp) here. I know the comps have 20px, but I feel like we need to keep to the vertical rhythm

  2. +++ b/css/src/components/breadcrumb.css
    @@ -26,6 +28,11 @@
    +    content: ' \003e ';
    

    Agree with @Ujval Shah. We should use a CSS shape here IMHO (maybe show the corners of a box that is rotated 45deg?)

mherchel’s picture

+++ b/css/src/components/breadcrumb.css
@@ -26,6 +28,11 @@
+    color: #e7edf1;

This color (and the other) should be referenced in a variable: var(--color--gray-80). See https://git.drupalcode.org/project/olivero/-/blob/8.x-1.x-dev/css/src/ba...

andrewmacpherson’s picture

Review of patch #3.

  1. + color: #e7edf1;

    The separator chevrons are VERY pale. #f7f9fa on a white background has a contrast ratio of just 1.05:1. (See https://contrast-ratio.com/#%23f7f9fa-on-white)

    If we want this to be recognized and understood as a breadcrumb, the separators need a stronger contrast.

    I'm unsure which WCAG success criterion the breadcrumb separators should conform to: 1.4.3 Contrast (minimum) or 1.4.11 Non-text contrast. Basically the question is whether these need a contrast ratio of 4.5:1 or 3:1. I'm leaning towards the lower non-text contrast, however I honestly don't know so I'll ask for a second opinion in the wider accessibility community, and report back.

    Try something like #949494, which provides exactly 3:1 contrast on a white background.

  2. + content: ' \003e ';

    Beware of punctuation characters in CSS content. These are conveyed to assistive technology (by design - it's treated as content). Some screen readers will announce the presence of a "greater" when moving through the breadcrumb in normal reading mode, but that's not what we actually mean here. See this detailed survey of which punctuation is announced by screen readers.

    Re. #9:

    We should use a CSS shape here IMHO (maybe show the corners of a box that is rotated 45deg?)

    Yes, I'd prefer that too, because it will be safer for screen readers. The rotated border trick will do for a simple chevron. Alternatively an SVG in CSS content: url() will work for any other shape.

  3. + li::before {
    This selector is too generic. We only want this in the breadcrumb.
anevins’s picture

It does make sense to enhance the contrast of the chevrons as we're using chevrons to convey information; to show the depth of the breadcrumb. As these chevrons are not just for decoration, I also recommend either making them 3:1 or 4.5:1 contrast at a minimum.

andrewmacpherson’s picture

Issue tags: +Accessibility

@anevins - thanks for your comment. The importance of the separators is certain.

What I'm less sure of is whether the separators should conform to SC 1.4.3, versus SC 1.4.11. Do you have any thoughts on that?

EDIT: oh, wait. I see you expressed a preference for SC 1.4.11 when we chatted on the web-a11y slack team. Let's see if there are any more responses to my support request there. So far 2 votes for SC 1.4.11 and no votes for SC 1.4.3

andrewmacpherson’s picture

Maintainers - can we get an issue credit for @anevins please? I chatted with them outside this issue queue, and they are helping to move #10.1 forward.

thejimbirch’s picture

Assigned: Unassigned » thejimbirch
thejimbirch’s picture

Assigned: thejimbirch » Unassigned
Status: Needs work » Needs review
FileSize
993 bytes

New patch added that updates 8.1, 8.2, 9, 11.1, 11.2, 11.3

11.1 I made the breadcrumb var(--color--gray-25) which deviates from the design, but is well over 4.5 contrast ratio.

11.3 - This compiles to .breadcrumb li::before

This does not address the mobile designb of the breadcrumbs. That needs work still. Not sure how that should be done.

andrewmacpherson’s picture

Further to #10.1 - I asked on the web-a11y Slack team for a second opinion on which WCAG contrast level should apply to the chevron separators:

  • SC 1.4.3 "Contrast (minimum)" used for text - 0 votes.
  • SC 1.4.11 "Non-text contrast" - 5 votes.

Outcome: the chevron separators need to have at least 3:1 contrast. It would be good to get the actual contrast comfortably over the threshold though. Any of the following colours from the existing palette will suffice:

  • --color--gray-20: #6E7172; provides 4.92:1 contrast against white.
  • --color--gray-25: #5D7585; provides 4.82:1 contrast against white. Used in patch #15.
  • --color--gray-30: #7E96A7; provides 3.08:1 contrast against white.
  • The palest pure-grey which provides 3:1 contrast against white is #949494.

@thejimbirch, re. #15:

This compiles to .breadcrumb li::before

Silly me. I was forgetting about PostCSS processing. Thanks for clarifying.

mherchel’s picture

Status: Needs review » Needs work

This patch needs to be re-rolled. Some nitpicks below:

  1. +++ b/css/src/components/breadcrumb.css
    @@ -7,10 +7,13 @@
    +  margin-bottom: var(--sp);
    

    You'll want to organize the properties per coding standards: https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

  2. +++ b/css/src/components/breadcrumb.css
    @@ -26,6 +29,19 @@
    +  li::before {
    

    Drupal does both ::before and :before styles. I don't know if there's official standards, but in Olivero, I'm just doing :before with a single quote.

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
25.63 KB
25.15 KB

No problems with desktop breadcrumbs, but that design for mobiles (with small shadow on right side + horizontal scroll) makes me sad a little bit.
Well, we can't add scrollable content, since actually it should be done by some js carousel library. For now, in this patch, i didnt add any js-library, but overflow-x: auto; was just added. Idk if it's enough or not.

Attaching some screens how that "carousel" and scroll-bar looks on my iphone:
Scrollable breadcrumbs on iPhone
Scrollable breadcrumbs on iPhone

mherchel’s picture

Status: Needs review » Needs work

Thanks for working on this! First pass is below :)

  1. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +  .grid-full & {
    

    No need for grid styles on components after https://www.drupal.org/project/olivero/issues/3123305 was merged in.

  2. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +  &__content {
    

    Let's not use this syntax (it's not used anywhere else in the theme). Write the full classname out, please.

  3. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +    border-bottom: 2px solid var(--color--gray-80);
    

    This border bottom should not be showing on wide widths.

  4. +++ b/templates/block/block--system-breadcrumb-block.html.twig
    @@ -0,0 +1,50 @@
    +{#
    

    Not sure if we need this template after the grid refactor was merged in.

mherchel’s picture

  1. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +    background: linear-gradient(to left, var(--color--white) 0%, rgba(255, 255, 255, 0) 100%);
    

    Add a comment indicating what this is (for those browsing through the code)

  2. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +      display: none;
    

    Instead of setting display: none, set content: none, to unset content.

  3. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +  &__link {
    

    Add a focus and hover states (can just be text-decoration: underline for now)

kostyashupenko’s picture

Assigned: Unassigned » kostyashupenko
kostyashupenko’s picture

Assigned: kostyashupenko » Unassigned
Status: Needs work » Needs review
FileSize
6.82 KB
8.01 KB
36.28 KB

Things are fixed, but few notes:

  1. +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +  .grid-full & {

    Transforms into

    +.grid-full .block-system-breadcrumb-block {
    +  @media (--lg) {
    +    grid-column: 3 / 15;
       }
     }

    We still need to override that setting for --lg breakpoint, please check the screen under (it comes from figma):

    Figma Breadcrumbs

    Right side of breadcrumbs is aligned with header's right side, so i decided to keep it like this.

  2. Also about that border:
    +++ b/css/src/components/breadcrumb.css
    @@ -6,27 +6,93 @@
    +    border-bottom: 2px solid var(--color--gray-80);
  3. Well, again, based on figma we should keep it always looks like. Biggest breakpoint we have is the following: @custom-media --xl (min-width: 1300px);, but desktop mockup on figma has 1440px width and border is there.

mherchel’s picture

Yeah, it's interesting. On the main page, I don't see an underline for the breadcrumbs, but on the breadcrumbs mockup, I do.

We'll leave it in for now, and adjust as necessary. I'll reach out to the designers for more clarification.

mherchel’s picture

Status: Needs review » Fixed

Committed! Thanks again!

I made a couple changes on commit:

- Changed the separator border color to --color--gray-30: #7E96A7, which has a 3.08 contrast ratio per @andrewmacpherson's comment in #12.
- Added a @todo to remove the layout logic from the CSS. I have a plan for that.

mherchel’s picture

I reached out to @jponch (one of the designers) and got the go-ahead to remove the border. He is also updating the mockup on Figma.

I agree [that the border doesn't look good when there's only one breadcrumb item]. Just wanted to make sure I wasn't missing what you were saying. I'd be down with ditching the border, since it becomes such a dominant visual element when it doesn't need to be. If we were having very little space or size shift between the breadcrumb and the title, then it could serve a real purpose. As it is we're using proximity and size contrast to clearly differentiate, and the border feels superfluous

andrewmacpherson’s picture

Re. #18:

I think the idea of using a scrolling overflow for breadcrumbs is bizarre; I don't recall ever seeing this anywhere in the wild. My hunch is that's likely to lead to discovery problems (especially on recent apple daft operating systems which go out of their way to hide scrollbars and make you guess which areas are scrollable). A more conventional approach is to let a breadcrumb run onto two lines where necessary, using the same line spacing as paragraph text. That way the full depth is always apparent.

Re. #20:

Add a focus and hover states (can just be text-decoration: underline for now)

Underline is a really poor focus indicator, because (a) it's what links normally look like in their non-focused state, and (b) goes against the established convention of surrounding controls on all 4 sides to indicate focus (it's the one thing all user-agent stylesheets agree on for focus styles).

Olivero already has a link style which is underlined in the non-focused state, and gains a 4 sided outline on focus. I strongly recommend making the breadcrumbs consistent with this. Having lots of different link styles and focus styles is going to make focus confusing to follow. (I have no opinion on hover styles for links, but note that the hover style for links elsewhere is an animated background colour which fills upwards.)

Re. #24:

- Changed the separator border color to --color--gray-30: #7E96A7, which has a 3.08 contrast ratio per @andrewmacpherson's comment in #12.

Um, I didn't actually make that recommendation. What I said in #16 was "It would be good to get the actual contrast comfortably over the threshold though". What I meant there, is that I liked the choice of --color--gray-25 used by thejimbirch in #15. I didn't say that very clearly though.

FWIW, I think the --color--gray-25 in #15 is a better choice; --color--gray-30 is barely over the threshold. Just because the minimum requirement is 3:1 doesn't mean we should deliberately aim that low.

Come to think of it, is there an explanation anywhere of the intended usage for the various colours in the palette? The list in variables.css doesn't say what they are for, and I can't see a palette in Figma.

mherchel’s picture

Um, I didn't actually make that recommendation.

This comment sounds super snarky and slightly condescending to me. Did you mean it to be? I might just be in a bad mood.

FWIW, I think the --color--gray-25 in #15 is a better choice; --color--gray-30 is barely over the threshold. Just because the minimum requirement is 3:1 doesn't mean we should deliberately aim that low.

I'm fine with that, and can make that change.

Olivero already has a link style which is underlined in the non-focused state, and gains a 4 sided outline on focus. I strongly recommend making the breadcrumbs consistent with this. Having lots of different link styles and focus styles is going to make focus confusing to follow. (I have no opinion on hover styles for links, but note that the hover style for links elsewhere is an animated background colour which fills upwards.)

I can see that point of view. My thought is we'll refine a number of these styles as we get closer to completion (and we can further abstract the styles, too).

Come to think of it, is there an explanation anywhere of the intended usage for the various colours in the palette?

Nope. That can be a followup issue if you'd like.

andrewmacpherson’s picture

Sorry - I didn't mean to sound snarky or condescending. I'm sure I do sound a bit pompous at times. I was trying to clarify what I thought about the stronger grey.

mherchel’s picture

No worries :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.