Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes a11y behaviour for reaction button and comment options button in an issue view #21743

Closed

Conversation

fsologureng
Copy link
Contributor

@fsologureng fsologureng commented Nov 9, 2022

Fixes #21742

  • Added aria markup to the add_reaction and context_menu of issue comments templates.
  • Added en_US locale text to comment context_menu button name.
  • Added divergent and simplified version of attachOneDropdownAria function as attachOneDropdownAriaTemplated for apply to dropdowns made by templates with aria markup. The new function only attach control functionality to the dropdowns.

@fsologureng fsologureng changed the title Fixes a11y behaviour for reaction button and comment options button in an issue view #21742 Fixes a11y behaviour for reaction button and comment options button in an issue view Nov 9, 2022
@techknowlogick
Copy link
Member

Thank you so much for this PR <3, I haven't tested it yet, but the CI is failing on some linting issues.

I copied the linting report below, but the originial can be found on drone. They follow the format: Line number, column, whether it is an error or a warning, a description, and finally an error code.

/drone/src/web_src/js/features/aria.js
   99:3   error    Expected an assignment or function call and instead saw an expression         no-unused-expressions
   99:21  warning  Unexpected console statement                                                  no-console
   99:33  error    Unexpected string concatenation                                               prefer-template
   99:68  error    Operator '+' must be spaced                                                   space-infix-ops
  107:5   error    Expected an assignment or function call and instead saw an expression         no-unused-expressions
  109:9   error    '$active' is never reassigned. Use 'const' instead                            prefer-const
  110:5   error    Expected space or tab after '//' in comment                                   spaced-comment
  117:5   error    Expected an assignment or function call and instead saw an expression         no-unused-expressions
  117:23  warning  Unexpected console statement                                                  no-console
  117:35  error    Unexpected string concatenation                                               prefer-template
  117:45  error    Operator '+' must be spaced                                                   space-infix-ops
  123:5   error    Closing curly brace does not appear on the same line as the subsequent block  brace-style

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 9, 2022
wolfogre and others added 3 commits November 9, 2022 17:22
Fix go-gitea#21698.

Set the last login time to the current time when activating the user
successfully.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again <3

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 10, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 10, 2022

TBH, I agree to improve the a11y. But the current approach doesn't look good to me. The code is just copied twice. I do not see a strong reason that why the old code couldn't be reused.

  • The performance: attachOneDropdownAria only changes the attributes which don't affect layout or rendering, so it could be fast enough.
  • For the worry that attachOneDropdownAria changes the DOM structure and it's slow: I think it's not a real problem. The attachOneDropdownAria also updates id and aria-controls. I do not see how the new function attachOneDropdownAriaTemplated make $dropdown.attr('aria-activedescendant', $active.attr('id')); work. I doubt that some screen readers may not work properly without these IDs.
  • From a development view, I do not think it's easy to make every developer(contributor) understand a11y clearly and write all dropdown elements with correct aria-attributes, and Fomantic UI Dropdown is quite special. In the end, attachOneDropdownAria is still widely used and it is a general solution, then attachOneDropdownAriaTemplated will become a one-time code just for the reactions. Then why not just keep one attachOneDropdownAria?

Just my opinion, not a blocker (if the code works)

@fsologureng
Copy link
Contributor Author

TBH, I agree to improve the a11y. But the current approach doesn't look good to me. The code is just copied twice. I do not see a strong reason that why the old code couldn't be reused.

Thanks for the review.
Actually, there is no compelling reason, it was just a proposal, would you prefer a sub-function dedicated only to adding control functionality? I'm open to doing it this way, but the proposed approach (probably poorly phrased, my fault) is not to leave 2 functions in production, but to make the whole change to the templates associated to the dropdowns so that only one remains.

* The performance: `attachOneDropdownAria` only changes the attributes which don't affect layout or rendering, so it could be fast enough.

The aria tree works as a layout too. Feel free to read some aspects in https://marcysutton.com/accessibility-and-performance

* For the worry that `attachOneDropdownAria` changes the DOM structure and it's slow: I think it's not a real problem. The `attachOneDropdownAria` also updates `id` and `aria-controls`. I do not see how the new function `attachOneDropdownAriaTemplated` make `$dropdown.attr('aria-activedescendant', $active.attr('id'));` work. I doubt that some screen readers may not work properly without these IDs.

Yes, it's my fault. In my haste I oversimplified the changes, I had made the addition of the ID to the templates, but in the end I made a reversal forgetting that action. Thanks for the review, I'll fix it. It won't work as it is.

* From a development view, I do not think it's easy to make every developer(contributor) understand a11y clearly and write all dropdown elements with correct aria-attributes, and Fomantic UI Dropdown is quite special. In the end, `attachOneDropdownAria` is still widely used and it is a general solution, then `attachOneDropdownAriaTemplated` will become a one-time code just for the reactions. Then why not just keep one `attachOneDropdownAria`?

As I wrote above, that is the main idea.

Just my opinion, not a blocker (if the code works)

Thank you (It needs a fix for the IDs)

@techknowlogick
Copy link
Member

@fsologureng mind if I send you an email to the one listed in your commits?

@fsologureng
Copy link
Contributor Author

@fsologureng mind if I send you an email to the one listed in your commits?
Please, feel free to do it.

lunny and others added 5 commits November 11, 2022 02:02
This is a performance regression from go-gitea#18058

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Andrew Thornton <art27@cantab.net>
A simple refactor to reduce duplicate codes.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: delvh <dev.lh@web.de>
Close go-gitea#21761

Some database migrations depend on the git module.
The purpose of go-gitea#18982 is to improve the SMTP mailer, but there were some
unrelated changes made to the SMTP auth in
go-gitea@d60c438

This PR reverts these unrelated changes, fix go-gitea#21744
 * Incorporated avoidance of aria related markup to attachOneDropdownAria function
 * Added id to menuitem roles to fix aria-activedescendant state
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@ce5aafb). Click here to learn what that means.
The diff coverage is 39.02%.

@@           Coverage Diff           @@
##             main   #21743   +/-   ##
=======================================
  Coverage        ?   48.24%           
=======================================
  Files           ?     1031           
  Lines           ?   140574           
  Branches        ?        0           
=======================================
  Hits            ?    67820           
  Misses          ?    64645           
  Partials        ?     8109           
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
routers/web/admin/auths.go 46.35% <0.00%> (ø)
routers/web/admin/config.go 23.17% <0.00%> (ø)
routers/web/auth/linkaccount.go 0.00% <0.00%> (ø)
routers/web/auth/oauth.go 28.12% <0.00%> (ø)
routers/web/auth/openid.go 0.00% <0.00%> (ø)
services/auth/source/smtp/auth.go 0.00% <0.00%> (ø)
services/auth/source/smtp/source.go 14.28% <ø> (ø)
services/auth/source/smtp/source_authenticate.go 0.00% <0.00%> (ø)
services/forms/auth_form.go 100.00% <ø> (ø)
... and 10 more

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

@silverwind
Copy link
Member

Latest merge seems to have gone wrong.

fsologureng and others added 8 commits November 11, 2022 11:13
- Update the crypto dependency to include
golang/crypto@6fad3df
- Resolves go-gitea#17798

Executed: `go get
golang.org/x/crypto@6fad3dfc18918c2ac9c112e46b32473bd2e5e2f9 && rm
go.sum && go mod tidy`
)

The `getPullRequestPayloadInfo` function is widely used in many webhook,
it works well when PR is open or edit. But when we comment in PR review
panel (not PR panel), the comment content is not set as
`attachmentText`.

This commit set comment content as `attachmentText` when PR review, so
webhook could obtain this information via this function.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Previously, there were discussions on how to write certain plurals.
So, we explicitly document the special plurals to end the discussion.
wxiaoguang and others added 20 commits November 22, 2022 00:33
- It's possible that the `user_redirect` table contains a user id that
no longer exists.
- Delete a user redirect upon deleting the user.
- Add a check for these dangling user redirects to check-db-consistency.
add webpack export type check

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
go-gitea#19999 introduced a indirect dependency with a license that was not on
our allowlist yet which produced this warning during webpack:

````
WARNING in License: citeproc@2.4.62 has disallowed license CPAL-1.0 OR AGPL-1.0
````

I've added both licenses to the allowed list and made it so webpack will
now abort on such license errors so that we don't miss those next time.

Co-authored-by: John Olheiser <john.olheiser@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Followup to go-gitea#21784.

- Restore muted effect on timeline author and issuelist comment icon
- Remove whitespace inside shared user templates, fixing link hover
underline
- Use shared author link template more
- Use `bold` class instead of CSS
- Fix grey-light color being too dark on arc-green
- Add missing black-light color
- Fix issuelist progress bar color
- Fix various other cases of missing `.muted`

<img width="416" alt="Screenshot 2022-11-13 at 12 15 22"
src="https://user-images.githubusercontent.com/115237/201519497-1d4725c6-bc8b-47b5-9f68-1278ac9a8c92.png">
<img width="324" alt="Screenshot 2022-11-13 at 12 16 52"
src="https://user-images.githubusercontent.com/115237/201519501-c0d03700-f9af-4316-ab46-482f2c7c738b.png">
<img width="79" alt="Screenshot 2022-11-13 at 12 30 55"
src="https://user-images.githubusercontent.com/115237/201519502-46dc2d73-bbdf-4a2e-84d3-d2976f793163.png">
<img width="440" alt="Screenshot 2022-11-13 at 12 41 03"
src="https://user-images.githubusercontent.com/115237/201519876-ada33948-f84a-4aeb-a40d-5c873f9a49e9.png">
<img width="213" alt="Screenshot 2022-11-13 at 12 52 54"
src="https://user-images.githubusercontent.com/115237/201520291-a4d7238e-aeca-46c7-9008-8b644b1b676e.png">
<img width="208" alt="Screenshot 2022-11-13 at 12 56 16"
src="https://user-images.githubusercontent.com/115237/201520436-aa8ba109-b959-42fb-831a-021e806c7082.png">

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This PR adds a context parameter to a bunch of methods. Some helper
`xxxCtx()` methods got replaced with the normal name now.

Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Show which Chroma Lexer is used to highlight the file in the file
header. It's useful for development to see what was detected, and I
think it's not bad info to have for the user:

<img width="233" alt="Screenshot 2022-11-14 at 22 31 16"
src="https://user-images.githubusercontent.com/115237/201770854-44933dfc-70a4-487c-8457-1bb3cc43ea62.png">
<img width="226" alt="Screenshot 2022-11-14 at 22 36 06"
src="https://user-images.githubusercontent.com/115237/201770856-9260ce6f-6c0f-442c-92b5-201e5b113188.png">
<img width="194" alt="Screenshot 2022-11-14 at 22 36 26"
src="https://user-images.githubusercontent.com/115237/201770857-6f56591b-80ea-42cc-8ea5-21b9156c018b.png">

Also, I improved the way this header overflows on small screens:

<img width="354" alt="Screenshot 2022-11-14 at 22 44 36"
src="https://user-images.githubusercontent.com/115237/201774828-2ddbcde1-da15-403f-bf7a-6248449fa2c5.png">

Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: John Olheiser <john.olheiser@gmail.com>
Wechatwork webhook is sending the following string for pull request reviews:

``` markdown
# 
>
```

This commit fixes this problem.
…21831)

The [labels in issue YAML
templates](https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms#top-level-syntax)
can be a string array or a comma-delimited string, so a single string
should be valid labels.

The old codes committed in go-gitea#20987 ignore this, that's why the warning is
displayed:

<img width="618" alt="image"
src="https://user-images.githubusercontent.com/9418365/202112642-93dc72d0-71c3-40a2-9720-30fc2d48c97c.png">

Fixes go-gitea#17877.
Fixes go-gitea#20514
Fixes go-gitea#20766
Fixes go-gitea#20631

This PR adds Cleanup Rules for the package registry. This allows to
delete unneeded packages automatically. Cleanup rules can be set up from
the user or org settings.
Please have a look at the documentation because I'm not a native english
speaker.

Rule Form

![grafik](https://user-images.githubusercontent.com/1666336/199330792-c13918a6-e196-4e71-9f53-18554515edca.png)

Rule List

![grafik](https://user-images.githubusercontent.com/1666336/199331261-5f6878e8-a80c-4985-800d-ebb3524b1a8d.png)

Rule Preview

![grafik](https://user-images.githubusercontent.com/1666336/199330917-c95e4017-cf64-4142-a3e4-af18c4f127c3.png)

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
## Changes proposed in [referenced issue 21845][1]

- Expand PAM configuration description with working examples.
- Clarify `STATIC_URL_PREFIX` use (include "assets" and only works after
database has been initialized)
- Add note for HTTPS proxy support VIA Apache.

[1]: go-gitea#21845
Also, run it via exact version instead of relying on global binary.
This patch provide a mechanism to disable RSS/Atom feed.

Signed-off-by: Xinyu Zhou <i@sourcehut.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: 6543 <6543@obermui.de>
I don't see why we have to use two versions of yaml. The difference
between the two versions has nothing to do with our usage.
It now supports copying Markdown, SVG and Images (not in Firefox
currently because of lacking
[`ClipboardItem`](https://developer.mozilla.org/en-US/docs/Web/API/ClipboardItem)
support, but can be enabled in `about:config` and works). It will fetch
the data if in a rendered view or when it's an image.

Followup to go-gitea#21629.
Embed the SVG icon directly, making further invertion unnecessary
because the icon color can now follow text color.

<img width="240" alt="Screenshot 2022-11-21 at 20 16 32"
src="https://user-images.githubusercontent.com/115237/203142189-89f20de9-c0bd-4d05-92c0-44dadf20d78f.png">
<img width="245" alt="Screenshot 2022-11-21 at 20 16 46"
src="https://user-images.githubusercontent.com/115237/203142191-658239ba-1859-49c6-91ad-10ddf14780d0.png">
- Update all JS deps
- Regenerate SVGs
- Add new eslint rules, fix issues
- Tested Mermaid, Swagger, Vue, Webpack, Citation

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
My pull request changes the logging documentation that is visible here:
https://docs.gitea.io/en-us/logging-configuration/
The reason behind the changes is that for some time I've found the
logging documentation confusing, and wanted to give a try at making it
more clear.

---

If you find the existing changes to be ok, please don't merge yet, as I
have further ideas which I want to discuss with you before making the
changes.

### Swap the "Log Groups" and "Log outputs" sections.
I want to move the "Log outputs" section before the "Log Groups"
section. The reason is that the "Log Groups" section refers to ini
sections that are only later explained, and to concepts that are general
and should be documented in "Log outputs" or a different section.

This change is essentially a swap of the "Log Groups" and "Log outputs"
sections. That way the doumentation would follow the structure in which
the ini file is built: first explaining the outer sections, and then the
inner ones ([log], [log.name], [log.name.default], ...)

### Explain the workings of ambigous settings below the settings listing
Right now the basics of a setting is shown later than the explanation of
its special workings, for example with `FILE_NAME` at [the file output
mode](https://docs.gitea.io/en-us/logging-configuration/#file-mode)
(well, if the first changes are taken into account).

Currently I have `TODO` witten at 2 settings, which I have to figure out
how do they exactly work before I can document them.

### New section about [log]
New section after "Collecting Logs for Help" about how the top level
[log] itself works and what can go there.
Currently, variables that directly go into [log] are noted throughout
the whole document.

---

Please let me know what you think about the changes.

A counterargument that I myself see is that some of this is already
present in the cheatsheet, but I think it would be better to have [this
document](https://docs.gitea.io/en-us/logging-configuration/) as a
throrough explanation of how logging is configured, and the cheatsheet
would only have a short outline of the possible sections and variables.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@lafriks
Copy link
Member

lafriks commented Nov 22, 2022

I think something is broken with this PR branch 🤔

fsologureng and others added 5 commits November 22, 2022 11:09
Standardize UTC timezone for `translateMonth` and `translateDay` tests.
Fix go-gitea#20456

At some point during the 1.17 cycle abbreviated refishs to issue
branches started breaking. This is likely due serious inconsistencies in
our management of refs throughout Gitea - which is a bug needing to be
addressed in a different PR. (Likely more than one)

We should try to use non-abbreviated `fullref`s as much as possible.
That is where a user has inputted a abbreviated `refish` we should add
`refs/heads/` if it is `branch` etc. I know people keep writing and
merging PRs that remove prefixes from stored content but it is just
wrong and it keeps causing problems like this. We should only remove the
prefix at the time of
presentation as the prefix is the only way of knowing umambiguously and
permanently if the `ref` is referring to a `branch`, `tag` or `commit` /
`SHA`. We need to make it so that every ref has the appropriate prefix,
and probably also need to come up with some definitely unambiguous way
of storing `SHA`s if they're used in a `ref` or `refish` field. We must
not store a potentially
ambiguous `refish` as a `ref`. (Especially when referring a `tag` -
there is no reason why users cannot create a `branch` with the same
short name as a `tag` and vice versa and any attempt to prevent this
will fail. You can even create a `branch` and a
`tag` that matches the `SHA` pattern.)

To that end in order to fix this bug, when parsing issue templates check
the provided `Ref` (here a `refish` because almost all users do not know
or understand the subtly), if it does not start with `refs/` add the
`BranchPrefix` to it. This allows people to make their templates refer
to a `tag` but not to a `SHA` directly. (I don't think that is
particularly unreasonable but if people disagree I can make the `refish`
be checked to see if it matches the `SHA` pattern.)

Next we need to handle the issue links that are already written. The
links here are created with `git.RefURL`

Here we see there is a bug introduced in go-gitea#17551 whereby the provided
`ref` argument can be double-escaped so we remove the incorrect external
escape. (The escape added in go-gitea#17551 is in the right place -
unfortunately I missed that the calling function was doing the wrong
thing.)

Then within `RefURL()` we check if an unprefixed `ref` (therefore
potentially a `refish`) matches the `SHA` pattern before assuming that
is actually a `commit` - otherwise is assumed to be a `branch`. This
will handle most of the problem cases excepting the very unusual cases
where someone has deliberately written a `branch` to look like a `SHA1`.

But please if something is called a `ref` or interpreted as a `ref` make
it a full-ref before storing or using it. By all means if something is a
`branch` assume the prefix is removed but always add it back in if you
are using it as a `ref`. Stop storing abbreviated `branch` names and
`tag` names - which are `refish` as a `ref`. It will keep on causing
problems like this.

Fix go-gitea#20456

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
It now supports copying Markdown, SVG and Images (not in Firefox
currently because of lacking
[`ClipboardItem`](https://developer.mozilla.org/en-US/docs/Web/API/ClipboardItem)
support, but can be enabled in `about:config` and works). It will fetch
the data if in a rendered view or when it's an image.

Followup to go-gitea#21629.
@fsologureng
Copy link
Contributor Author

Closed in favor of #21964

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The add reaction button and the comment options button are not accessible in the web view of an issue