Dialog focus improvements #473
Conversation
samuelpecher
left a comment
samuelpecher
left a comment
There was a problem hiding this comment.
Nice! I think we need to move some code around close to the the appropriate place.
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
2 times, most recently
from
1e7b94e to
9549e2e
Compare
|
@samuelpecher would love your input on these changes 🙏 |
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
2 times, most recently
from
f00e783 to
a8bfb16
Compare
There was a problem hiding this comment.
This is a great improvement to the toolbar DOM, and any interaction we can drop is always a win. 🎉
I think the toggle/open/close situation can have clearer responsibilities and I've got a couple more nits.
I'll drop the state update callback mechanism and roll it up into a sigh, beforetoggle handler<details> doesn't have that event~~
samuelpecher
force-pushed
the
dialog-focus-improvements
branch
from
a388ebb to
5781f6f
Compare
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
from
cf8c4bf to
fbd2b87
Compare
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
3 times, most recently
from
ac3c73c to
4d3c8f3
Compare
Focus on first interactive element on open Dialogs renamed to dropdown CSS fixes Test failure fixes
The `required` attribute will be added on toggle
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
from
4d3c8f3 to
d7cf4f0
Compare
| if (newState === "open") { | ||
| this.editor.getEditorState().read(() => { | ||
| this.#updateColorButtonStates($getSelection()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Just a general thought: I removed the callbacks yet I don't love a toolbar summary reaching into the editor like this. It's the trade-off for not scaffolding a whole series of callbacks.
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
from
d7cf4f0 to
dfbf103
Compare
zoltanhosszu
force-pushed
the
dialog-focus-improvements
branch
from
dfbf103 to
17496b6
Compare
|
@jorgemanrubia would you mind taking a look at these changes? 🙏 |
Dialog to details
For the two existing toolbar dialogs, this PR changes their structure to
<details>, similar to the overflow menu. This enables us to remove a lot of extra JS code that came with using<dialog>.Tab indexes
Changed how tabindexes are assigned. Instead of giving them all a unique value, they all get 0, as per MDN specs. This simplifies the code but still follows the visual layout of the elements. Plus with the above change, the open dropdowns are now logically placed in the tab order.