Skip to content

Correct minor regression from #1158; add tabindex for consistency.#1168

Merged
demiankatz merged 5 commits into
UniversalViewer:devfrom
rNegi-bl:dev
Oct 30, 2024
Merged

Correct minor regression from #1158; add tabindex for consistency.#1168
demiankatz merged 5 commits into
UniversalViewer:devfrom
rNegi-bl:dev

Conversation

@rNegi-bl

@rNegi-bl rNegi-bl commented Oct 23, 2024

Copy link
Copy Markdown
Contributor

Follow-up to #1158.

@vercel

vercel Bot commented Oct 23, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 11:57am

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @rNegi-bl, this looks like progress, but it's unfortunately still not quite working for me all the time.

Here's something I'm seeing in Firefox:

1.) I go to the preview and hit tab until I focus the image adjustment button. The button remains focused and visible -- this is what I expect.

2.) Now I hit shift-tab until I focus away from the image controls, so the image controls disappear.

3.) After this point, if I tab forward to the image adjustment control again, the image controls disappear even though the control is focused.

So it seems like some state is getting lost over time after multiple interactions. Can you investigate? Please let me know if you need more help!

@thattonBL

Copy link
Copy Markdown
Contributor

Thanks, @rNegi-bl, this looks like progress, but it's unfortunately still not quite working for me all the time.

Here's something I'm seeing in Firefox:

1.) I go to the preview and hit tab until I focus the image adjustment button. The button remains focused and visible -- this is what I expect.

2.) Now I hit shift-tab until I focus away from the image controls, so the image controls disappear.

3.) After this point, if I tab forward to the image adjustment control again, the image controls disappear even though the control is focused.

So it seems like some state is getting lost over time after multiple interactions. Can you investigate? Please let me know if you need more help!

Hi Damien, I am struggling to recreate the Firefox issue. I am on Windows and Firefox 131.0.3 (64-bit). What's your system and browser?

@demiankatz

demiankatz commented Oct 23, 2024

Copy link
Copy Markdown
Contributor

Hi Damien, I am struggling to recreate the Firefox issue. I am on Windows and Firefox 131.0.3 (64-bit). What's your system and browser?

I have an identical system: Windows 11 + the same 64-bit Firefox 131.0.3.

I also reproduced the same behavior using the same sequence of actions in Chrome 130.0.6723.59 (64-bit), so the problem doesn't seem to be browser-specific.

In case it makes a difference, here's my exact series of keypresses to trigger the condition:

Hit tab 15 times, wait (image doesn't fade -- expected behavior), hit shift-tab 5 times, hit tab 5 times, wait (image does fade -- problem behavior).

@thattonBL

Copy link
Copy Markdown
Contributor

Hi Damien, I am struggling to recreate the Firefox issue. I am on Windows and Firefox 131.0.3 (64-bit). What's your system and browser?

I have an identical system: Windows 11 + the same 64-bit Firefox 131.0.3.

I also reproduced the same behavior using the same sequence of actions in Chrome 130.0.6723.59 (64-bit), so the problem doesn't seem to be browser-specific.

In case it makes a difference, here's my exact series of keypresses to trigger the condition:

Hit tab 15 times, wait (image doesn't fade -- expected behavior), hit shift-tab 5 times, hit tab 5 times, wait (image does fade -- problem behavior).

Thanks, yes I can recreate it following these steps.

@rNegi-bl

rNegi-bl commented Oct 23, 2024

Copy link
Copy Markdown
Contributor Author

Hi Damien, controlsFadeDelay & controlsFadeLength set for the viewer is fading out the controls. If I give max values to those properties it will keep AdjustImageButton & other controls visible & they won't fade out. Will that be correct ?

@demiankatz

Copy link
Copy Markdown
Contributor

Hi Damien, controlsFadeDelay & controlsFadeLength set for the viewer is fading out the controls. If I give max values to those properties it will keep AdjustImageButton visible & they won't fade out. Will that be correct ?

I think the question (which I don't know the answer to) is why this is only a problem for the image adjust button and not the others. Is it possible there is logic attached to those other buttons but not to this one, and things need to be made more consistent?

Your proposed solution may be the best way forward -- I'd just like to be sure we don't treat this button differently from the rest if we don't have to. :-)

Let me know if you need more help researching this; I can take a longer look at the code later today if necessary.

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the progress, @rNegi-bl -- I think there's a new issue now, though: once you have focused the adjust controls, then the fade is disabled forever. I think we need to remember the previous values and restore them on blur.

@demiankatz demiankatz changed the title AdjustImageButton visibility correction (fixes #1076) Correct minor regression from #1158; add tabindex for consistency. Oct 30, 2024

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some overlapping work in #1174 has addressed the adjust image button issue in a way that addresses my comments here, so I have reverted the focus handler from this PR so that we can merge the other parts of the work here that correct other issues. I think we can rely on #1174 to fix the remainder of the issue.

@demiankatz demiankatz merged commit 6e69a1a into UniversalViewer:dev Oct 30, 2024
@rNegi-bl

Copy link
Copy Markdown
Contributor Author

Hi Demian, this merge will close #1076 ?

@demiankatz

Copy link
Copy Markdown
Contributor

Hi Demian, this merge will close #1076 ?

@rNegi-bl, to fully resolve #1076, we need to merge #1174, which addresses the last detail of making the image adjustment button behave correctly. It looks like that PR has just been approved, so I should be able to merge it and close the issue soon.

Thanks for all of your work on this, and sorry that it got a little bit confusing! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Completed

Development

Successfully merging this pull request may close these issues.

3 participants