Block Gallery: deselect first/last image on blur #14930
Conversation
oandregal
requested review from
ajitbohra,
gziolo,
jorgefilipecosta,
noisysocks,
notnownikki,
Soean,
talldan and
youknowriad
as code owners
oandregal
changed the title
oandregal
added
[Block] Gallery
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
90c6591 to
d99971e
Compare
| // | ||
| // onBlur / onFocus events are quick operations (<5ms apart in my testing), | ||
| // so 50ms accounts for 10x lagging while feels responsive to the user. | ||
| this.debouncedOnDeselect = debounce( this.props.onDeselect, 50 ); |
There was a problem hiding this comment.
I still don't get from this comment why focus on the same element would be called right after blur. In what circumstances does rapid focus & blur happen?
There was a problem hiding this comment.
Updated the comment to better explain the mechanism: the idea is to call onDeselect when focus has transitioned to a different React component (another GalleryImage, toolbar, etc.) but not when focus transitions within the DOM elements that form this React component (figure, image, caption, toolbar, etc).
Does it make more sense now?
There was a problem hiding this comment.
@ellatrix any chance you'd be able to review this and see if the comment makes more sense now?
There was a problem hiding this comment.
Can't we instead just check that the window.activeElement property is not a children of the wrapper (using a ref) instead of this magic trick :)
There was a problem hiding this comment.
It's just code! Perhaps the comment is more confusing than the code itself? Suggestions welcome :)
I had tried the comparison with the activeElement, but the global focus handler won't be reliably dispatched, so I resorted to the focus events dispatched by the children instead.
There was a problem hiding this comment.
I still don't get from this comment why focus on the same element would be called right after blur. In what circumstances does rapid focus & blur happen?
Now that I revisit this thread I think I can explain this better! It's not that rapid focus and blur happen on the same element, but that focus and blur on children element bubble to figure. In the DOM, blur and focus don't bubble, but React Events mechanism normalizes all events to bubble - even blur and focus. So this is how events work in gallery:
- user clicks
image=> focus event is fired with image as target then it bubbles to figure - user tabs to
figcaption=>- blur event is fired for
imageas target, then it bubbles tofigure - focus event is fired for
figcaption, then it bubbles tofigure
- blur event is fired for
And the same thing for the GalleryImage buttons (icons to remove or move the image). So, instead of tracking blur events individually, this PR tracks them in a single place.
Also worth noting that if you check the activeElement in a blur event it'll be body in Firefox and Chrome (it doesn't give you the next element to focus). So, the only alternative I see would be to listen to focus events at the window level, and check if the activeElement is one of the GalleryImage children. This doesn't work because the focus won't bubble in some situations.
There was a problem hiding this comment.
@nosolosw Could you do something like event.currentTarget.contains( event.relatedTarget ) in the onBlur to detect when focus moved outside a gallery image?
There was a problem hiding this comment.
That would work and it seems to have good browser support. However, the feature is marked as experimental and is only in a working draft. My gut feeling is that we shouldn't use it just yet, what do you think?
|
I tested and if an image is selected, after tabbing away, the image deselects. Thanks @nosolosw! |
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
0533054 to
a198701
Compare
|
Would be good to revisit this at some point, it looks like a simple quick fix. |
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
a198701 to
abe4412
Compare
|
I've rebased this and it still works as expected. @youknowriad the alternative approach you mentioned (to hook into the window focus event and compare the activeElement to all the children of GalleryImage) didn't work for me. For some reason, the focus event isn't reliably dispatched. I also think this is more performant and simpler than having to track refs for all children. |
|
@ellatrix do you think you can help review this PR :) |
|
cc @youknowriad @ellatrix bumping visibility for this. |
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
abe4412 to
fcbb7c6
Compare
|
This has been rebased with the latest changes in master and this is ready for review. |
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
fcbb7c6 to
970f5dd
Compare
|
Rebased to fix code formatting issues (presumably after the prettier merge). |
|
I have tested this on mobile, and I have not encountered any regressions. 👍 |
|
@youknowriad @talldan @ellatrix @jorgefilipecosta how do you feel about this PR code-wise? Should we go ahead with this implementation or should we close it for now and revisit later? |
|
I think we should move forward, I'd have preferred a simpler fix but we can go with that one for now. |
|
ok, so this would need a 👍 then. |
|
When tabbing backward, the image controls (remove, reorder buttons) are skipped entirely, is that expected? |
oandregal
force-pushed
the
fix/gallery-image-deselect
branch
from
806dda7 to
92a87e4
Compare
|
FYI: I've just rebased this from
@youknowriad That's the behavior on |
|
Size Change: +146 B (0%) Total Size: 866 kB
ℹ️ View Unchanged
|
Fixes #14823