Fix hidden commit status on multiple checks

[oliverpool]

Since #22632, when a commit status has multiple checks, no check is shown at all (hence no way to see the other checks).

This PR fixes this by always adding a tag with the .commit-statuses-trigger to the DOM (the .vm is for vertical alignment).

Cc @fsologureng @zeripath @lafriks who were involved into the previous PR.

Contributed in the context of @forgejo

[oliverpool]

1. I added a test to ensure that the commit-statuses-trigger is present when multiple checks are present.
   This requires different contexts, hence the change in the last argument of doAPICreateCommitStatus.

2. If there is no status, no HTML is shown at all (wrapped the whole template in if - sorry for the unreadable diff)

[fsologureng]

The template fix looks good to me because removing the status and the popup in case of an empty repository (which is the original case (bad)handled) is even better.

About the tests part I won't pronounce because I am not familiar with them.

[oliverpool]

I think I found an almost elegant way to deal with link/span:

• either a link is shown: when eq (len .Statuses) 1 then the first (and unique) status is already available in .Status and a .Status.TargetURL can be made
• or a span with tabindex="0" to support for touch users and keyboard navigation

Both use the data-tippy="commit-statuses" attribute.

---

It makes visually no difference: the top one is a a, the bottom is a span:

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@ca445cf). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main   #22889   +/-   ##
=======================================
  Coverage        ?   47.36%           
=======================================
  Files           ?     1121           
  Lines           ?   150300           
  Branches        ?        0           
=======================================
  Hits            ?    71197           
  Misses          ?    70660           
  Partials        ?     8443           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Ryuno-Ki]

Thanks for picking up my suggestion! I went through the code and left some comments.

[fsologureng]

@oliverpool as I said in @forgejo's devel chat; the only problem that persists, and to be fair it wasn't the aim of this fix or mine, is that the tippy list opened with hover event in desktop environment doesn't work in mobile environment; the problem is that the hover doesn't exist in mobile, so to open the status list you need to tap on the anchor, so it triggers both the navigation to the url and show the list.

The solution should separate the hover event to the click event, probably maintaining the tippy trigger whenever, but with an additional anchor for the status url. Or some similar arrangement.

Thanks for picking up my review :)

[oliverpool]

@fsologureng you are right. But if there is only one check, with a target URL, visiting this URL is not that bad.

However it is true that having a variable behavior (depending on things invisible to the user: one or more statuses) is not the best UX. But as you noted the goal of this PR is not to make a perfect UX, but fix a UX bug (slightly improve the UX :).

[wxiaoguang]

Reply to chat:

Friendly ping for a 2nd maintainer review for ...

Although I am not maintainer, I can see some divergences from your side. Do you have a final consensus?

• If no, nobody knows what to do. Do you still need to improve the a11y?
• If yes, tell the maintainers that "OK, no more discussion, no more change".

ps: Personally, in my option, I do not think using data-tippy=xxx pattern is the generally used method in Gitea. A js-xxx selector is shorter and clearer since you only want to bind an event handler to it to show tippy, just my opinion:

• The data-xxx attributes are usually used to store some data, eg: the relation between elements. Its value usually provides related information, but is not just used to be a selector.
• The guideline also suggests to use js- prefix, and it has already been used in many places in Gitea.
• When people read data-xxx they will pay more time to think and search what's the meaning of the value. When people read js-xxx they know immediately: oh, that's a selector for JS.

[oliverpool]

@wxiaoguang thank you for your comment.

Do you have a final consensus?

The original goal of this PR is to fix a regression. As such this PR is complete.

Accessibility (a11y) still isn't optimal, however achieving an optimal state would mean deeper changes (in appearance and functionality). Since 1.19 should be released soon, I think this PR should be merged as-is and a follow-up PR could be made to further improve this.

The guideline also suggests to use js- prefix, and it has already been used in many places in Gitea.

Which guideline are you referring to? I have looked at https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md, but wasn't able to find a recommended way.

I think creating a dedicated Styleguide.md could really help to clear such situations upfront (#22879 with gt- prefixes could also be documented there).

---

Edit: I just saw #22847 (comment)

Markup the templates with non-tailwind helpers having g-h- prefix and tailwind classes with a tw- prefix and then get it to a point where we can drop fomantic. Consider using js- class for hooks for javascript or id where appropriate and stick gitea- prefixes on specific components.

(However it is only the opinion of @zeripath, having this formalized in a accessible markdown document would show that it is agreed upon).

[zeripath]

@oliverpool we went for gt- in the end over g-h- as it's easier to read, but my idea still stands and I think we should do it if just because the UI state is a bit of a mess right now.

[oliverpool]

I think we should do it

I also think so. And documenting it in the repo would greatly help. STYLEGUIDE.md for instance? (with a link from CONTRIBUTING.md)

Otherwise it is only known to people involved in the discussion and might cause avoidable friction during PR-reviews.

[wxiaoguang]

Do you have a final consensus?

As such this PR is complete.

It's good to have a consensus. Otherwise I guess reviewers may worry about "If I approve this, would someone thinks that I do not understand a11y or do not care about a11y?"

And:

• reviewing PRs loosely may introduce more problems (like a11y, like 22632)
• reviewing PRs strictly may introduce more discussions and pend more PRs.

So, at the moment, Gitea's reviewing is not too loose nor too strict, that's why sometimes a few bugs come, sometimes there are a few a11y problems, and sometimes there are some pending PRs. That's a balance and a trade-off.

About Styleguide.md, about js-, about gt-

There are documents (I always write documents and comments)

• https://github.com/go-gitea/gitea/blob/main/docs/content/doc/developers/guidelines-frontend.en-us.md
• https://docs.gitea.io/en-us/guidelines-frontend/
• https://github.com/go-gitea/gitea/blob/main/web_src/less/helpers.less (the .gt-hidden part)

Although a few of the them might be out-dated, most of them are still helpful.