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: cell loses focus when clicking on its outline #3185

Closed

Conversation

manthan-jsharma
Copy link
Contributor

@manthan-jsharma manthan-jsharma commented Aug 28, 2023

Fixes #2987

Technical details

rectifying the issue with fixating upon the blue and grey box shadow by making it inset

Screenshots

Screenshot 2023-08-28 at 5 06 40 PM

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Sorry, something went wrong.

@seancolsen seancolsen self-assigned this Aug 28, 2023
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Aug 28, 2023
@rajatvijay rajatvijay added this to the Backlog milestone Aug 29, 2023
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

@manthan-jsharma

This change is not acceptable because it modifies the visual design of the active cell indicator.

Before After
image image

Before your change, the blue outline lay outside the cell rectangle. After your change, the blue outline lies inside the cell rectangle.

We want to preserve the visual design we already have.

@seancolsen seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Sep 8, 2023
@manthan-jsharma
Copy link
Contributor Author

Hi @seancolsen,

Based on my understanding, there appear to be two issues:

(1) The first issue arises when a user clicks between the edge of the cell and the box-shadow, causing the cell to lose focus. This behavior is not a bug but rather the default behavior of the browser, and it is consistent across various websites such as GitHub and YouTube.

bug reproduce

Untitled-2

github

Untitled-3

youtube

Untitled-4

(2) The second issue occurs when a user clicks at the edge of the box-shadow, causing focus to shift to another cell.

bug reproduce

Untitled

If my interpretation is correct, it seems that the second issue is the one we should consider a bug or unexpected behavior.

If we consider the second issue as the primary bug, here is a proposed solution: We can apply two types of box shadows, one with a 1px inset shadow and the other with a 1px outset shadow. This approach can help prevent the occurrence of this bug.

With the addition of both inset and outset shadows, the behavior changes in a way that prevents focus from shifting to another cell. This is because the box shadow does not extend beyond the border.

behavior after inset and outset shadows

Untitled-5

code changes for 2nd issue

 &.is-active {
      box-shadow: inset 0 0 0 1px var(--slate-300), 0 0 0 1px var(--slate-300);
      border-radius: 2px;
      &.is-focused {
        box-shadow: inset 0 0 0 1px var(--sky-700), 0 0 0 1px var(--sky-700);
      }
    }

@seancolsen, I'd like to hear your thoughts on whether we should address the second issue. If we prioritize the first issue, please share any potential solutions you have in mind.

@seancolsen
Copy link
Contributor

@manthan-jsharma No, this issue is actually not about either 1 or 2, as you describe.

Let me take a step back and explain why this is important...

When a cell element has focus, many different keyboard shortcuts are active. For example, I can use arrow keys to move the active cell. Our UX design is such that, when the cell loses focus, those keyboard shortcuts are no longer active. This UX design has some drawbacks but it somewhat important for usability and accessibility throughout the rest of the app (e.g. within the UI of the table inspector). This UX design is also consistent with Google Sheets, for example. The drawback is that it becomes quite important to ensure that the user doesn't inadvertently steal focus away from the cell. If they do, then we could be interrupting their work, requiring more mouse usage and slowing things down. Due to these requirements, it should be the case the clicking anywhere within the sheet is a safe operation focus-wise. That is, there should be clickable areas within the sheet that steal focus from cells.

This issue describes a very thin area between cells which steals focus from cells. If I click click on a horizontal line between two cells, which cell should be selected? The top one? The bottom one? It doesn't matter! The important thing is that some cell should be selected and focused, otherwise we have an area within the sheet that steals focus from cells.

Does this make sense?

@manthan-jsharma
Copy link
Contributor Author

manthan-jsharma commented Sep 25, 2023

Hi @seancolsen i got the issue now, whenever i click on border line of unselected cell, it doesn't get focus.(border is that thin line)

As a solution, I suggest adding an "onclick" event to the cell border so that it can get focus. Could you please confirm if we can proceed with this solution?

@seancolsen
Copy link
Contributor

@manthan-jsharma I'd rather not add more event listeners to handle this. We currently have several event listeners implemented per cell, and in the long term we'd actually like to remove those for performance reasons. I would imagine that we'd be able to fix this issue with the proper markup and CSS. The active cell border should be visual only — no behavior, so no JS.

@seancolsen
Copy link
Contributor

@manthan-jsharma I'll leave this PR open for another couple weeks and then close it if we haven't heard from you.

@manthan-jsharma
Copy link
Contributor Author

As you have already mentioned the specific direction you want this change to be proceeded so that it does not cause any redundancy on the established designs and code structures associated to it, i have thought of working it out with a pseudo element layer above the shadow box or something along the line(i will be extrapolating it further) , that would potentially pertain the click event to be always at call whenever its needed. Consequently, if a click occurs on the specific edges we've introduced, the event and the associated focus will be maintained.

@seancolsen
Copy link
Contributor

@manthan-jsharma from your last comment I'm not sure exactly what you're proposing, but I'll be happy to review your next iteration of it once I see the code.

@seancolsen
Copy link
Contributor

@manthan-jsharma are you still working on this? I'll close it if we don't see any continued work here in a couple weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Cell loses focus when clicking on its outline
3 participants