Correct minor regression from #1158; add tabindex for consistency.#1168
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
demiankatz
left a comment
There was a problem hiding this comment.
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? |
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. |
|
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 ? |
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
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! :-) |
Follow-up to #1158.