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

Set autocomplete off on branches selector #15809

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 9, 2021

Fix #15782

Signed-off-by: Andrew Thornton art27@cantab.net

Fix go-gitea#15782

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/bug topic/ui Change the appearance of the Gitea UI backport/v1.14 labels May 9, 2021
@zeripath zeripath added this to the 1.15.0 milestone May 9, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2021

Codecov Report

Merging #15809 (a2dce38) into main (2dc3e4e) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #15809      +/-   ##
==========================================
+ Coverage   43.92%   44.00%   +0.08%     
==========================================
  Files         681      681              
  Lines       82228    82228              
==========================================
+ Hits        36120    36187      +67     
+ Misses      40205    40133      -72     
- Partials     5903     5908       +5     
Impacted Files Coverage Δ
modules/git/repo_commit.go 40.00% <0.00%> (-0.87%) ⬇️
models/error.go 39.84% <0.00%> (+0.31%) ⬆️
services/pull/pull.go 43.37% <0.00%> (+0.45%) ⬆️
models/notification.go 66.07% <0.00%> (+0.88%) ⬆️
modules/notification/notification.go 87.60% <0.00%> (+2.47%) ⬆️
modules/notification/base/null.go 78.94% <0.00%> (+2.63%) ⬆️
modules/notification/mail/mail.go 38.77% <0.00%> (+5.10%) ⬆️
models/issue_comment.go 52.58% <0.00%> (+5.89%) ⬆️
modules/notification/ui/ui.go 65.81% <0.00%> (+6.83%) ⬆️
modules/util/timer.go 85.71% <0.00%> (+42.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc3e4e...a2dce38. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 9, 2021
@silverwind
Copy link
Member

silverwind commented May 9, 2021

Hmm I think we rather want to prevent the element from getting autofocus, autocomplete may still be useful.

@zeripath
Copy link
Contributor Author

zeripath commented May 10, 2021

Hmm I think we rather want to prevent the element from getting autofocus, autocomplete may still be useful.

This is precisely how fomantic's dropdown.js prevents this box from appearing.

if( module.is.search() && !module.has.search() ) {
module.verbose('Adding search input');
$search = $('<input />')
.addClass(className.search)
.prop('autocomplete', 'off')
.insertBefore($text)
;
}

@jtran
Copy link
Contributor

jtran commented May 10, 2021

Hmm I think we rather want to prevent the element from getting autofocus, autocomplete may still be useful.

IMHO, the dropdown is a custom implementation of autocomplete. Having both a custom implementation and the browser's is conflicting. On the compare/New Pull Request page, you can select a branch by using the arrow keys to navigate the dropdown. Unfortunately, the branch selector in this pull request uses a separate implementation and doesn't have the cursor movement functionality. But presumably, they'll get unified at some point.

Anyway, for what it's worth, I think this is a good change.

@silverwind
Copy link
Member

I can't seem to reproduce the original issue, whatever I do, it won't show any dropdown over the input. Tested in Firefox 89 and Chromium 90.

@jtran
Copy link
Contributor

jtran commented May 10, 2021

For me, the browser's autocomplete shows in Firefox 88.0.1 if I click on the input, after it already has the focus. So the steps are: click the branch selector (the dropdown should open and the input should have the focus), then click the input. I don't know if there are other ways to trigger it.

@zeripath
Copy link
Contributor Author

I can't seem to reproduce the original issue, whatever I do, it won't show any dropdown over the input. Tested in Firefox 89 and Chromium 90.

I can reliably reproduce the issue and my PR fixes the issue.

@silverwind
Copy link
Member

silverwind commented May 10, 2021

So the steps are: click the branch selector (the dropdown should open and the input should have the focus), then click the input.

Strange, doing that in Firefox 89 on Mac just brings up the browser-native dropdown menu with a input history, which I don't find too distracting. This behaviour might actually be platform-dependant.

@jtran
Copy link
Contributor

jtran commented May 11, 2021

Strange, doing that in Firefox 89 on Mac just brings up the browser-native dropdown menu with a input history...

I could be mistaken, but I thought that's what this issue is. My browser input history is short, so it doesn't look like the issue screenshot, but if it were longer, I think it would be the same. I'm also running on mac.

@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 May 11, 2021
@zeripath
Copy link
Contributor Author

Yes that is the problem - it very much gets in the way, you can't get rid of it, it's inconsistent with the rest of the drop-downs and it's really quite unhelpful.

@silverwind
Copy link
Member

It doesn't bother me really and it's very hard to accidentially trigger it but I guess it is the correct solution to hide it.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 11, 2021
@techknowlogick techknowlogick merged commit fc6501e into go-gitea:main May 11, 2021
@zeripath zeripath deleted the fix-15782-autocomplete-off-on-branch branch May 11, 2021 17:23
zeripath added a commit to zeripath/gitea that referenced this pull request May 11, 2021
Backport go-gitea#15809

Fix go-gitea#15782

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@zeripath zeripath added the backport/done All backports for this PR have been created label May 11, 2021
zeripath added a commit that referenced this pull request May 11, 2021
Backport #15809

Fix #15782

Signed-off-by: Andrew Thornton <art27@cantab.net>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
Fix go-gitea#15782

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overlapped branches list on project view
6 participants