Skip to content

Add "dir=auto" for input/textarea elements by default #26735

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Sep 7, 2023
Merged

Add "dir=auto" for input/textarea elements by default #26735

merged 19 commits into from
Sep 7, 2023

Conversation

wxiaoguang
Copy link
Contributor

No description provided.

@wxiaoguang wxiaoguang added the topic/ui Change the appearance of the Gitea UI label Aug 25, 2023
@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Aug 25, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 25, 2023
@wxiaoguang wxiaoguang mentioned this pull request Aug 25, 2023
Closed
@wxiaoguang

This comment was marked as outdated.

Sign in to view
@silverwind
Copy link
Member

Should we filter to exclude type hidden and checkbox? I don't see an issue adding it to them, but it might trigger obscure bugs in obscure browsers.

@silverwind
Copy link
Member

Will this work on both static and js-injected content?

@wxiaoguang
Copy link
Contributor Author

Will this work on both static and js-injected content?

Yes, just like the tooltip, it works for Vue

@silverwind
Copy link
Member

Can keep without the filter as well I guess, it will be slightly faster without.

@wxiaoguang
Copy link
Contributor Author

Can keep without the filter as well I guess, it will be slightly faster without.

Didn't see any noticeable difference ..... I guess keeping the :not([dir]) is good, in case some elements already have their own dir.

@silverwind
Copy link
Member

silverwind commented Aug 25, 2023 •
edited
Loading

Did a few tweaks and also added some temporary benchmark logging. What worries me is that in Chrome, this takes 10-20ms, in Firefox 5-10ms just on a simple issue list page. Note that this is a cost paid on each page load.

image

Maybe we should instead of installing multiple MutationObservers only install a single one that does everything?

@silverwind
Copy link
Member

silverwind commented Aug 25, 2023 •
edited
Loading

Imho the perf impact of this is too much. I can easily see this observer taking 1-2 seconds of time on a large diff page for example which would certainly freeze the browser. The info mentioned here is very much relevant, but none of those tricks really help. One should just not run querySelectorAll inside a observer callback, at least not synchronously.

I think #26715 is the better solution that can not possibly have such a potentially huge perf impact like this has.

silverwind
silverwind requested changes Aug 25, 2023
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Perf impact too big

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 25, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 26, 2023 •
edited
Loading

There are still some key points, eg: for of is slower than for(i++) (tested by the empty loops).

And another point is: it doesn't need to be executed in the <head>, it could also be executed "on DOM loaded"

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 26, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 26, 2023 •
edited
Loading

Fully rewritten, now it takes around 1.5ms on a PR page with +6966 -3615 (the rendered changed lines is around 3000)

@wxiaoguang wxiaoguang requested a review from silverwind August 26, 2023 02:33
@GiteaBot GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Aug 26, 2023
@silverwind
Copy link
Member

silverwind commented Sep 6, 2023 •
edited
Loading

Switched to Set, it is faster at least with 5 elements.

@silverwind
Copy link
Member

Another small gain made with length caching.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Sep 6, 2023
@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 Sep 7, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 7, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Sep 7, 2023 •
edited
Loading

Switched to Set, it is faster at least with 5 elements.

I am sure the "perf.link" is somewhat/somehow buggy. It shows my code is faster too. While such minor performance doesn't bother me, either is fine.

Firefox (the first is Set)

image

Chrome (the second is Set)

image

@puni9869
Copy link
Member

puni9869 commented Sep 7, 2023

Let's roll 🚀

@silverwind
Copy link
Member

I am sure the "perf.link" is somewhat/somehow buggy. It shows my code is faster too. While such minor performance doesn't bother me, either is fine.

At least Chrome and Safari show a significant improvement for Set. In Firefox it is closer for me too, but still 5-10% faster.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Sep 7, 2023 •
edited
Loading

I am sure the "perf.link" is somewhat/somehow buggy. It shows my code is faster too. While such minor performance doesn't bother me, either is fine.

At least Chrome and Safari show a significant improvement for Set. In Firefox it is closer for me too, but still 5-10% faster.

How do your browser think about "#26735 (comment)" ? On my side, Set is significantly slower.

The "perf.link" is just a blackbox, I don't fully trust it for some critical tasks.

(I just would like to clarify the problem, again: While such minor performance doesn't bother me, either is fine.)

@silverwind
Copy link
Member

How do your browser think about "#26735 (comment)"

Firefox

by set: 2.589832999976352, by equal: 2.4636940001510084
by set: 2.9552920000860468, by equal: 0.8551479999441653
by set: 1.521970999892801, by equal: 0.7742269999580458

Chrome

by set: 42.900000005960464, by equal: 5.200000002980232
by set: 28.200000002980232, by equal: 1.1000000089406967
by set: 16.399999991059303, by equal: 0.5

@wxiaoguang
Copy link
Contributor Author

How do your browser think about "#26735 (comment)"

Chrome

by set: 42.900000005960464, by equal: 5.200000002980232
by set: 28.200000002980232, by equal: 1.1000000089406967
by set: 16.399999991059303, by equal: 0.5

That's a huge different result from perf.link. I think our manually written test code is more like the real case we are facing.

@silverwind
Copy link
Member

Switched it back

@silverwind silverwind enabled auto-merge (squash) September 7, 2023 07:49
@silverwind silverwind merged commit 1221221 into go-gitea:main Sep 7, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 7, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 8, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
820ff43 
* giteaofficial/main:
  Add `yamllint` (go-gitea#26965)
  Fix yaml quoting (go-gitea#26964)
  [skip ci] Updated translations via Crowdin
  Add `actions/labeler` (go-gitea#26962)
  Team invite url fix when registration disabled (go-gitea#26950)
  Refactor dashboard/feed.tmpl (go-gitea#26956)
  Improve hint when uploading a too large avatar (go-gitea#26935)
  Replace `util.SliceXxx`  with `slices.Xxx`  (go-gitea#26958)
  Add reverseproxy auth for API back with default disabled (go-gitea#26703)
  Add "dir=auto" for input/textarea elements by default (go-gitea#26735)
@wxiaoguang wxiaoguang deleted the dir-auto branch September 11, 2023 15:47
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@silverwind silverwind silverwind approved these changes

@puni9869 puni9869 puni9869 approved these changes

Assignees
No one assigned
Labels
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
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wxiaoguang @silverwind @puni9869 @lunny @GiteaBot