a11y Merge Request Widget

changed milestone to %12.9

added devopsverify grouppipeline security typefeature + 1 deleted label

added [deprecated] Accepting merge requests label

mentioned in issue #25566 (closed)

marked this issue as related to #25566 (closed)

added to epic &1301 (closed)

mentioned in epic &2021 (closed)

changed epic to &2021 (closed)

changed milestone to %12.10

added GitLab Premium direction labels

added workflowneeds issue review label

@jheimbuck_gl - this is a neat issue. I would like to hear more about:

Why is this GitLab Premium? Is is because director's care about a11y testing in general and will actually be the ones consuming this?
Where will this interaction with Sasha really occur? Is it in the IU and where? Is it in the CLI? If so, what is the first step to getting the interaction between the MR and a11y results?
Where else could these results be useful? As in, are there items in other stages - like the Design group that would find Web Accessibility results interesting?
Can you elaborate more on what this will accomplish from a use case view? Are we saving time for the developer? "Shifting Left" the devops cycle? Increasing Quality? Delivering better compliance as a part of improved time to market?

added workflowissue reviewed label and removed workflowneeds issue review label

added workflowstart label

Why is this GitLab Premium? Is is because director's care about a11y testing in general and will actually be the ones consuming this?

I was thinking about visual review tools which is in Premium being a requirement but it's not, review apps is which is in Core. I'll revise.

Where will this interaction with Sasha really occur? Is it in the IU and where? Is it in the CLI? If so, what is the first step to getting the interaction between the MR and a11y results?

In this iteration it is in the MR as a widget much like code quality. I'll get the design links added to help tell that story.

Where else could these results be useful? As in, are there items in other stages - like the Design group that would find Web Accessibility results interesting?

Designers would very likely find this interesting, maybe @cdybenko would be interested as well.

Can you elaborate more on what this will accomplish from a use case view? Are we saving time for the developer? "Shifting Left" the devops cycle? Increasing Quality? Delivering better compliance as a part of improved time to market?

Shifting left. The use case is more well defined in the MVC but i'll copy over here and call out the case of "I want to see how my changes impacted a11y before merge, so i can fix them without deploying the full app and testing manually"

@jheimbuck_gl @jmeshell

From a Design Management perspective, I was thinking of taking the Design Tab to Core and then the Premium upgrade would be things like visual review and post-handoff communication where the designs are present for inspection. See some of the paid options in this space are in this competitive analysis doc: https://docs.google.com/document/d/1sW6Wxmb_RPlHKi06phfqiV_aAZ4TUbNZtY_EnJGvMVU/edit

I'm still open to it being Core, but it would be a neat carrot as well if we could nail the visual inspection of designs against the review app.

I think this aligns with that vision @cdybenko. More features targeted at Managers/directors for both visual review tools and a11y are likely to be in Starter or above.

Preliminary Design for a11y MR Widget:

Maybe we can do more than this, but putting them here for discussion:

Not sure if this is existing copy for other widgets, but "for the source branch only" seems ambiguous to me. My first read was that only the source branch was analyzed and 10 issues were found on it.

Something like "no new issues" or "0 issues introduced in the changes" might be better. Something that explicitly states that we're talking about a diff.

I think this is existing copy @drewcimino but great point and we may want to change that across the board.

@drewcimino This is the existing copy for other widgets, but I agree. We should change it in the others as well.

@jj-ramirez can you update design with a suggestion and / or @parker_ennis or @marcel.amirault may be able to suggest something that's more clear for us?

@drewcimino's suggestions sound applicable. "No new issues introduced in the changes" is clear to me. Are we only talking about the most recent submitted change(s)? That would be the only other part I'm curious about -- if we should clarify it's only referring to the most recent proposed change(s)

@drewcimino @jheimbuck_gl @parker_ennis

I created a separate issue for this: #214566 (closed)

I can address this change. This is quite small and easy. Can we please confirm if Parker's suggestion makes sense?

@parker_ennis to answer your question, I'm quite sure that this is referring to the current MR changes only. Specifically, the diff.

Thanks for circling back here @jj-ramirez! Reading again, it occurred to me that "issues" could potentially be confusing since we use the terms issues/merge requests so frequently in other contexts. The suggestion as is still makes perfect sense and shouldn't be confusing to users, however we could consider using "problems" or "discrepancies" instead (although "discrepancies" sounds a little too formal). Just a thought!

removed GitLab Premium label

changed the description

added workflowplanning breakdown label and removed workflowstart label

added 1 design

changed the description

added UX documentation labels

mentioned in issue #207959 (closed)

mentioned in issue #118856 (closed)

assigned to @drewcimino

removed [deprecated] Accepting merge requests label

added VerifyP1 label

@jheimbuck_gl JSON a11y artifacts are currently ~"workflow::In dev" #118856 (closed), but this will also require some backend to be able to compare the reports and come up with diff data.

@iamricecake and @morefice I started looking at TestReportsComparerEntity as model for "AccessibilityReportsComparerEntity", and was thinking it's generic enough that we might be able to consider the exposed fields there a general API for compared reports. Same with TestSuiteComparerEntity; it's sort of a "count of bad things, count of new bad things (positive or negative). WDYT of using the same classes versus a near-copy?

@drewcimino @iamricecake

I think it would be more explicit to make a separate report for it. This will make the code very clear about its original intent. Sometimes duplication is necessary for better readability/simplicity.

What are the pros and cons to create a different one for our new accessibility reports?

I would like us to take simple decisions, and later if we find too much duplication we can abstract it at a higher level. Thoughts?

That sounds right to me, we should leave code duplication concerns out of scope until the feature exists.

This might be simpler anyway because we don't really need the TestReports layer, as a11y is essentially a single suite with a list of failures.

changed the description

assigned to @zeffmorgan

added frontend label

added backend label

@shampton As discussed, could you lead us towards any MRs or Issues that mirrors the proposed frontend constructs to complete this? Thanks!

@dcipoletti here is the MR for making the MR widget for generic metrics. The implementation for this should be similar.

Thanks, @shampton! This looks like a great way to get kickstarted :)

added GitLab Starter label

added GitLab Core label and removed GitLab Starter label

assigned to @shampton

mentioned in issue #211389 (closed)

removed backend label

@jheimbuck_gl @erushton I've split off the backend work required to support this widget into a separate issue, with a plan for implementation and testing with acceptance criteria. I've marked that as Deliverable since the proposal and scope both seem very reasonable to me.

@mfluharty @shampton I think this issue can be marked as Deliverable if that backend issue looks like it'll have everything you need for this, which will become the frontend and feature issue.

I've applied a weight of 2 to this, per the frontend folks in the call last Thursday

@drew thanks for splitting of the backend into a different issue. I think the frontend just needs to know the data schema returned from the backend.

I'd imagine something like

{
  numberOfIssues: <int>
  issues: [{
    status: ('warning' or 'error')
    message: <string>
  }]
}

The status property would be used for the icon on the left of each issue. I'm not sure if we should separate out the message into different parts based on the design. Maybe you understand more of what the backend will be generating than I do.

What do you think?

@shampton I think we'll use the same structure as TestSuiteComparerEntity; in this ecosystem I think an a11y scan is equivalent to a test suite, and would come back in a TestReport alongside rpsec, junit, etc. (They wouldn't actually be side-by-side by default, but structurally they'd be at the same level and it would be possible to configure them that way.

TestSuiteComparerEntity uses new_ existing_ and resolved_ prefixes, which I think we would keep. But rather than failures and errors, use errors, warnings, and notes.

@drewcimino I've been working on my code with some mock data. Could you take a quick look to make sure this is the format that I should expect to receive?

@shampton The responses structure looks right, but your individual records seem off. I'm looking at https://drewcimino.gitlab.io/-/a11y-scanning/-/jobs/475297041/artifacts/reports/accessibility.json, is there another reference where you saw what you're using there?

@drewcimino Yeah, I know the information is wrong. I was just putting stuff in there as a placeholder. Sorry for the confusion! I just wanted to make sure the structure was what you were expecting to send over.

changed weight to 2

marked this issue as related to #211389 (closed)

assigned to @jj-ramirez

added Deliverable workflowready for development labels and removed workflowissue reviewed workflowplanning breakdown labels

created merge request !27429 (closed) to address this issue

mentioned in merge request !27429 (closed)

added Kickoff Highlight label

started a discussion on 2.png

added workflowin dev label and removed workflowready for development label

started a discussion on 2.png

mentioned in issue #211954

mentioned in issue #211955

marked this issue as related to #207383

removed the relation with #211389 (closed)

marked this issue as related to #211389 (closed)

mentioned in merge request gitlab-com/www-gitlab-com!44550 (merged)

unassigned @drewcimino

assigned to @morefice

@shampton I was chatting with @morefice and he suggested that we might be able to ship a smaller MVC by doing the comparison in the front-end, the way we do with CodeQuality. Considering the eventual/possible size of the a11y artifacts this might not be a better long-term solution, but would it allow the widget to ship faster?

I can't remember if we discussed doing it this way and had a strong reason to not, initially.

@drewcimino I'm fine with doing the comparison on the frontend if that makes it easier to ship. I'll need to get an updated schema of what is being sent to the frontend though. Will it just be the same as https://drewcimino.gitlab.io/-/a11y-scanning/-/jobs/475297041/artifacts/reports/accessibility.json?

I can't remember what our discussion on this was either. I don't know if the size of these artifacts are going to be a major problem on an already slow MR page. Maybe @jheimbuck_gl has some better insight here.

@morefice and I have the same concern about artifact size, but if it's quick and not a lot of work to unwind later, we could always set up the service on the backend and switch the widget to that in the the next release.

@shampton and @drewcimino i'm ok moving fast and we can do some testing in staging to generate a really big report and see how it goes.

@jheimbuck_gl Thanks - I'll work on getting it parsed on the frontend. @drewcimino is the response just going to be the artifact data as found in https://drewcimino.gitlab.io/-/a11y-scanning/-/jobs/475297041/artifacts/reports/accessibility.json?

That it, out in the wild

@drewcimino @shampton

In the meantime, I'm trying to get an idea of how complicated the parsing will be on the backend. I will keep the schema as close as possible to the current one we have today.

Stay tuned for more update this week.

@drewcimino @morefice @jheimbuck_gl I'm working on parsing on the frontend and I have some questions on how these issues should be combined. If two issues have the code WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail, they can be combined to say found two errors of the following type: WCAG2AA.Principle1.Guideline1_4.1_4_3.G18.Fail. However, if the messages are different should we still combine them?

For example, in https://drewcimino.gitlab.io/-/a11y-scanning/-/jobs/475297041/artifacts/reports/accessibility.json we have one message say

This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 3.84:1. Recommendation:  change text colour to #767676.

and another with the same code say

This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 2.82:1. Recommendation:  change background to #d1470c."

If the messages are different, should we keep them separate? I'd imagine so, but I just want to make sure. The examples in the design are truncated to only show This element has insufficient contrast at this conformance level. so I wasn't sure if we just truncate all of the messages or if we show the whole thing. If we truncate them, then I guess we can just combine the messages if the truncated messages are equal.

What do you think?

That's a great question @shampton !

Looking at the design it suggests we should combine them. This would probably add some extra work for our MVC maybe we can keep it simple and not merging them yet?

What are your thoughts @jheimbuck_gl @jj-ramirez ?

Keep them separate for now if that's faster and create a follow-up issue to combine which we can get feedback on from users.

@shampton @morefice

Thanks @jheimbuck_gl. Just to clarify, are we separating all issues even if they have the same code and message or are we only separating issues that have different messages but combining those have the same code and message?

@shampton I can't recall if we're presenting a count anywhere off the top of my head to avoid confusion let's just separate everything for now and we can iterate on this after.

mentioned in merge request !28823 (merged)

removed the relation with #211389 (closed)

marked this issue as related to #211389 (closed)

Talking with @drewcimino, we realized we need to add the base path and head path for the accessibility reports in the app/serializers/merge_request_widget_entity.rb file similar to how the code quality report is done in the EE version of that file. This will probably need to be a separate MR.

mentioned in merge request !29199 (merged)

changed health status to at risk

mentioned in issue gl-retrospectives/verify#25 (closed)

mentioned in merge request !29965 (merged)

changed milestone to %13.0

added missed-deliverable missed:12.10 labels

changed health status to on track

Update

Following up our discussion in !29199 (comment 327449415)

We chatted with team if this was a good idea to display the URL of an accessibility error on our MR widget.

We are not planning to tackle it yet but we need to keep this requirement in mind for further iterations.

This will impact the backend schema to make a diff between 2 accessibility reports.

With URL

{ "https://gitlab.com" => [data1, data2] }

Without URL

[data1, data2]

cc @drewcimino @iamricecake @shampton @jj-ramirez @rickywiens

I'm in favor of keeping the URL involved. We can always concat the arrays and throw away the URLs if we don't want them, but we can't put them back.

Yes, let's keep the URL. This gives us more flexibility in the future.

Sounds good to me - let's keep the URL.

@morefice as I was working on updating the frontend to use the backend API endpoint, I realized I don't know how the schema is going to look with the URLs included. Will it look like this?:

{
  :status=>"failed",
  :new_errors=>{
    "https://gitlab.com"=>[
      {
        :code=>"WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.A.NoContent",
        :type=>"error",
        :type_code=>1,
        :message=>"Anchor element found with a valid href attribute, but no link content has been supplied.",
        :context=>"<a class=\"social-icon\" target=\"_blank\" href=\"https://gitlab.com\" rel=\"nofollow noopener noreferrer\">\n        <svg xmlns=\"http://www...</a>",
        :selector=>"html > body > header > div > nav > a",
        :runner=>"htmlcs",
        :runner_extras=>{}
      }
    ]
  },
  :resolved_errors=>[],
  :existing_errors=>{
    "https://gitlab.com"=>[
      {
        :code=>"WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.A.NoContent",
        :type=>"error",
        :type_code=>1,
        :message=>"Anchor element found with a valid href attribute, but no link content has been supplied.",
        :context=>"<a class=\"site-title active\" rel=\"author\" href=\"/\">\n        <svg version=\"1.0\" xml...</a>",
        :selector=>"html > body > header > div > nav > a",
        :runner=>"htmlcs",
        :runner_extras=>{}
      }
    ]
  },
  :summary=>{
    :total=>1,
    :resolved=>0,
    :errored=>2
  }
}

@shampton thanks for asking!

So the accessibility report includes the urls but they will not be exposed to the API yet for the sake of simplicity.

We could add them whenever we want when it's required and when we will iterate on this feature with a new design for it.

JSON schema


{
  :status=>"failed",
  :new_errors=>
  [
    {
      :code=>"WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.A.NoContent",
      :type=>"error",
      :type_code=>1,
      :message=>"Anchor element found with a valid href attribute, but no link content has been supplied.",
      :context=>"\n        ",
      :selector=>"html > body > header > div > nav > a",
      :runner=>"htmlcs",
      :runner_extras=>{}
    }
  ],
  :resolved_errors=>[],
  :existing_errors=>
  [
    {
      :code=>"WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.A.NoContent",
      :type=>"error",
      :type_code=>1,
      :message=>"Anchor element found with a valid href attribute, but no link content has been supplied.",
      :context=>"\n        ",
      :selector=>"html > body > header > div > nav > a",
      :runner=>"htmlcs",
      :runner_extras=>{}
    }
  ],
  :summary=>{
    :total=>1,
    :resolved=>0,
    :errored=>1
  }
}

@morefice Awesome, that sounds great. Thanks!

mentioned in issue #214566 (closed)

mentioned in issue #212499 (closed)

Update from frontend

I have an MR in maintainer review that finishes up the frontend work when the comparison is done on the client side. It is behind a feature flag, but wouldn't render anyway since the base_path and head_path for the accessibility report are not exposed currently.

Since the ~backend is close to being done doing the comparison server-side, we'll need to adjust the frontend slightly to get the report from the server. So there will be at least one more frontend MR to work on after the backend is complete.

mentioned in merge request !31487 (merged)

Update

I have another MR in review that switches the comparison to the backend endpoint. This will still be behind a feature flag, and also won't work until we actually have the endpoint path passed to the frontend. So once the backend work is completed we can followup to make sure the integration is seamless and make any changes if necessary.

I also have an MR for adding documentation that is in review right now, but won't be able to be merged until we actually get this feature working.

/cc @jheimbuck_gl @morefice

@shampton what's going to be the LAST MR that needs merged before this is live? That's the one I'd lie to reference in the release post MR so when it's merged we know to merge the release post content. Thanks!

@jheimbuck_gl That's a good question. It might be this current one that I have in review, but if that gets merged before the backend is finished then the last one will be whichever one the backend finishes last .

@morefice what do you think?

@shampton - Can you create an issue to disable the feature flag that relates any pending WIP and we'll use it lacking a better process for now?

cc @rickywiens

@jheimbuck_gl - do you mean create an issue to remove the feature flag? The feature flag is already disabled and hiding the widget. I just want to clarify before creating the issue.

Sorry, wasn't clear @shampton . An issue to flip the flag to whatever state is needed to make this feature show up after the last MR is merged and closed. We can do another issue to remove it and always have this on.

@jheimbuck_gl Created the issue: #217372 (closed)

@shampton @jheimbuck_gl

@morefice what do you think?

It seems that the last MR wrapping up this feature will be the backend endpoint.

Thanks @shampton and @morefice for the update. Since that MR you mentioned is merged can this be merged now as well?

@jheimbuck_gl @morefice I realized we need to expose the accessibility endpoint path so that the frontend has it. I'll create an MR now and ping @morefice to review. Should be really small.