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

fix: fix select all action always select the editor content #415

Merged
merged 5 commits into from
Sep 9, 2021

Conversation

mortalYoung
Copy link
Collaborator

@mortalYoung mortalYoung commented Sep 8, 2021

简介

  • 修复全选总是会选中 editor 内容的 bug

主要变更

  • menu 组件内监听 focusin 的事件,并且 menu 新增 focusinEle 属性,用于存储 menu 打开之前的保持 focus 的节点
  • 移除 id 为 molecule 的节点的 tabindex,这个节点太过接近 body,导致每次点击都只能拿到这个节点,会影响判断
  • action 的 run 逻辑优化。
    • 目前通过 menu 点击执行命令的话,会传入一个参数,该参数为 menu 打开之前的 focus 的节点
    • 当不存在该参数,则表示是通过快捷键执行全选,那就可以通过 document.activeElement 取到当前 focus 的元素,然后去执行该元素的 select 方法或者 monacoselectAll 方法
    • 如果存在该参数,表示是通过点击菜单执行的全选,那该参数就是在打开菜单之前 focus 的节点,则对该节点执行 select 方法或者 monacoselectAll 方法

Related Issues

Closed #326

@mortalYoung mortalYoung self-assigned this Sep 8, 2021
@mortalYoung mortalYoung added the bug Something isn't working label Sep 8, 2021
@mortalYoung mortalYoung added this to the 0.9.0-beta.1 milestone Sep 8, 2021
@mortalYoung mortalYoung requested a review from wewoor September 8, 2021 08:38
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #415 (ecee9ed) into main (5e68b1f) will decrease coverage by 0.20%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   81.27%   81.06%   -0.21%     
==========================================
  Files         177      177              
  Lines        4849     4875      +26     
  Branches     1044     1051       +7     
==========================================
+ Hits         3941     3952      +11     
- Misses        899      914      +15     
  Partials        9        9              
Impacted Files Coverage Δ
src/monaco/quickSelectAllAction.ts 27.27% <0.00%> (-39.40%) ⬇️
src/controller/menuBar.ts 37.73% <50.00%> (+1.00%) ⬆️
src/workbench/menuBar/menuBar.tsx 86.66% <100.00%> (+3.33%) ⬆️
src/services/workbench/editorService.ts 97.92% <0.00%> (+0.02%) ⬆️

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 5e68b1f...ecee9ed. Read the comment docs.

@mortalYoung
Copy link
Collaborator Author

覆盖率差的比较大的原因是因为 action 的 run 逻辑加了很多分支判断,所以判定下来低了很多

Copy link
Collaborator

@wewoor wewoor left a comment

Choose a reason for hiding this comment

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

关闭 Editor 后,如果不 focus 到输入控件,快捷键就无效了

@mortalYoung
Copy link
Collaborator Author

关闭 Editor 后,如果不 focus 到输入控件,快捷键就无效了

那肯定无效呀 0.0 只有 Editor 是作为兜底聚焦的存在,如果连 Editor 都没有打开,那正常来说就是什么都不发生,并不是快捷键无效

@wewoor
Copy link
Collaborator

wewoor commented Sep 9, 2021

关闭 Editor 后,如果不 focus 到输入控件,快捷键就无效了

那肯定无效呀 0.0 只有 Editor 是作为兜底聚焦的存在,如果连 Editor 都没有打开,那正常来说就是什么都不发生,并不是快捷键无效

像切换 Theme, 和 Display Language 都是不需要依赖 Editor 聚焦的

@mortalYoung
Copy link
Collaborator Author

关闭 Editor 后,如果不 focus 到输入控件,快捷键就无效了

那肯定无效呀 0.0 只有 Editor 是作为兜底聚焦的存在,如果连 Editor 都没有打开,那正常来说就是什么都不发生,并不是快捷键无效

像切换 Theme, 和 Display Language 都是不需要依赖 Editor 聚焦的

解决了,快捷键失效的原因是因为 id 是 molecule 的 div 的 tabindex 是不可以去掉,快捷键的触发依赖于这个元素的聚焦

Copy link
Collaborator

@wewoor wewoor left a comment

Choose a reason for hiding this comment

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

+1

@wewoor wewoor merged commit a1feb18 into main Sep 9, 2021
@wewoor wewoor deleted the fix/select-all branch September 9, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select All action works global
2 participants