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

Ensure that sidebar field will not steal focus when metadata is edited #5983

Merged
merged 20 commits into from
Jun 21, 2024

Conversation

dobri1408
Copy link
Contributor

When editing metadata externally, as in the case of our EEA metadata block, the current functionality causes the sidebar to automatically gain focus. However, this behavior is unnecessary in this context and results in the sidebar capturing focus inappropriately. To address this, I propose a solution where the sidebar will only receive focus if the user is actively working within it. This can be achieved by checking the activeElement to determine if the sidebar should be focused.

screen-capture.webm

Copy link

netlify bot commented Apr 25, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit dfadc5e
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66752bed09af390008b120c9

Copy link

netlify bot commented Apr 25, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 6f4ce67
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/663cca49ce37a0000808bd67

@sneridagh
Copy link
Member

@dobri1408 are you sure that this does not break the original intention of the code (#4230). Apparently, seems it does not, but still.

@dobri1408
Copy link
Contributor Author

dobri1408 commented Apr 25, 2024

@sneridagh, @tiberiuichim @ichim-david , I believe there might be a misunderstanding. Consider this perspective: the field in question is displayed in the sidebar, as indicated by the className being sidebar-metadata. Therefore, if we are not operating within the sidebar context, it seems unnecessary to focus on these fields. Also it can be a potential bug, as I have shown you. My condition ensures we check whether we are in the sidebar or not.

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

LGTM, although I'd like to have feedback from David and Tiberiu.

@tiberiuichim
Copy link
Contributor

Seems strange passing react props derived from the state of the DOM elements in the browser

@ichim-david ichim-david marked this pull request as draft April 26, 2024 09:48
@ichim-david
Copy link
Member

@sneridagh I'm putting the pull request in draft until we have a better look at the problem and consider if we need to handle it from Volto core.
This is a problem of our doing within our add-on https://github.com/eea/volto-metadata-block/ that allows you to mirror the content type fields from the sidebar to the content area. Because it renders with the same id and widget when there is a field with a react-select and given the field id it focuses on the sidebar which is bad if you interact with the widget in the content area.

Given Tiberiu's comment even if we do propose something for the core we might need todo it differently so for now we treat it as review given and we still have some work todo.

@dobri1408 dobri1408 marked this pull request as ready for review May 8, 2024 13:05
@dobri1408
Copy link
Contributor Author

After conducting research and consulting with my colleagues, we have concluded that the focus management approach used by the Select Widget is incorrect. As demonstrated in the attached video, the issue does not originate from our metadata-section-block.

The problem originates from this particular pull request: https://github.com/plone/volto/pull/4003/files#diff-e739dae43ff2e2a3a433411ca92aa574aa0502d440e3734905df9e1b96522ebfR318

My solution, as outlined in this pull request, addresses all these issues. I suggest we reconsider this approach and possibly enhance it if it doesn't meet your expectations.

screen-capture.1.mp4

We've had this bug for quite some time. As you can see here, we were forced to implement customizations because of it: https://github.com/eea/volto-cca-policy/tree/master/src/customizations/volto/components/manage/Widgets

@dobri1408 dobri1408 requested a review from sneridagh May 8, 2024 13:17
@dobri1408
Copy link
Contributor Author

dobri1408 commented May 22, 2024

Hello @ichim-david, @tiberiuichim, @sneridagh! I wanted to highlight that the issue is present in every Volto project, including demo.plone.org. This problem is specific to Volto and is not related to the EEA addons. I've added a cypress test and uploaded a video to clearly demonstrate the issue. Could you please review it when you have a moment? Thank you!

screen-capture.1.1.mp4

@ichim-david
Copy link
Member

@dobri1408 simply add the test as a new it test here: https://github.com/plone/volto/blob/main/packages/volto/cypress/tests/core/basic/metadata.js there is no need to have a separate file which isn't a .js file and which isn't testing the FileWidget.

@sneridagh
Copy link
Member

@ichim-david @dobri1408 is it ready to merge?

Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@sneridagh yes it's ready for merging

@dobri1408
Copy link
Contributor Author

Hello @sneridagh, could you, also, backport this to Volto 17?

@sneridagh sneridagh merged commit 50bba48 into main Jun 21, 2024
70 checks passed
@sneridagh sneridagh deleted the fix-field-sidebar-stealing-focus branch June 21, 2024 09:39
sneridagh added a commit that referenced this pull request Jun 26, 2024
* main: (73 commits)
  Release 18.0.0-alpha.36
  Rename missing command
  Image widget PR as breaking (#6125)
  Release @plone/slate 18.0.0-alpha.14
  Release @plone/registry 1.7.0
  Rename Makefile targets (#6104)
  fix: nonContentRoutes diff path (#6102)
  Automatically set the label to `03 type: feature (plip)` for PLIPs (#6122)
  Add ImageWidget with upload/drop/external and inline/widget capabilities (#5607)
  Ensure that sidebar field will not steal focus when metadata is edited (#5983)
  Prevent duplicated UUUIDs in inner blocks when copying container blocks (#6112)
  feat: handle breakList in detached TextBlockEditor (#6106)
  Make dynamic/live teaser without additional requests (#6024)
  Fix issue of duplicated blocks upon pasting an image into the Slate E… (#5818)
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants