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

Visual Editor: Hide sibling inserter by CSS #3503

Merged
merged 1 commit into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Nov 15, 2017

Fixes #3408
Context: #3408 (comment)
Cherry-picks cefa3aa from #3502 (otherwise untestable)

This pull request seeks to simplify the rendering implementation of VisualEditorSiblingInserter to avoid manually handling rendering of the child Inserter component, instead allowing the inserter to render and visual appearance by CSS styling. This resolves issues where the Inserter was not appearing when tabbing in IE11. Some manual management of inserter visibility is still required, specifically when focus or cursor positioning moves within the inserter while active.

Testing instructions:

Verify that there are no regressions in the behavior of between-inserters.

Repeat steps to reproduce from #3408, verifying that the between-inserter can be activated via keyboard.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 15, 2017

Codecov Report

Merging #3503 into master will increase coverage by 0.1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3503     +/-   ##
=========================================
+ Coverage   36.45%   36.56%   +0.1%     
=========================================
  Files         267      267             
  Lines        6652     6632     -20     
  Branches     1207     1203      -4     
=========================================
  Hits         2425     2425             
+ Misses       3570     3554     -16     
+ Partials      657      653      -4
Impacted Files Coverage Δ
editor/components/block-list/sibling-inserter.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2d4f64...d442fa6. Read the comment docs.

codecov bot commented Nov 15, 2017

Codecov Report

Merging #3503 into master will increase coverage by 0.1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3503     +/-   ##
=========================================
+ Coverage   36.45%   36.56%   +0.1%     
=========================================
  Files         267      267             
  Lines        6652     6632     -20     
  Branches     1207     1203      -4     
=========================================
  Hits         2425     2425             
+ Misses       3570     3554     -16     
+ Partials      657      653      -4
Impacted Files Coverage Δ
editor/components/block-list/sibling-inserter.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2d4f64...d442fa6. Read the comment docs.

@aduth aduth requested a review from afercia Nov 21, 2017

@aduth aduth merged commit da8aadb into master Nov 21, 2017

3 checks passed

codecov/project 36.56% (+0.1%) compared to f2d4f64
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Nov 22, 2017

Contributor

Sorry for being late. Tested on IE11 and also on a few browser/screen reader combos and it works nicely: the sibling inserter appears and is announced correctly.

However, when tabbing with a keyboard, the transition that reveals the inserter button is very slow, I guess it's the sum of the delay + the transition and... with a keyboard the tooltip appears immediately. So there's a noticeable delay from the moment the tooltip appears and when the button appears:

screen shot 2017-11-22 at 14 42 13

This doesn't happen when hovering, only when focusing the button. @jasmussen is the transition really necessary? Or maybe make it a bit faster, maybe?

Contributor

afercia commented Nov 22, 2017

Sorry for being late. Tested on IE11 and also on a few browser/screen reader combos and it works nicely: the sibling inserter appears and is announced correctly.

However, when tabbing with a keyboard, the transition that reveals the inserter button is very slow, I guess it's the sum of the delay + the transition and... with a keyboard the tooltip appears immediately. So there's a noticeable delay from the moment the tooltip appears and when the button appears:

screen shot 2017-11-22 at 14 42 13

This doesn't happen when hovering, only when focusing the button. @jasmussen is the transition really necessary? Or maybe make it a bit faster, maybe?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 23, 2017

Contributor

This doesn't happen when hovering, only when focusing the button. @jasmussen is the transition really necessary? Or maybe make it a bit faster, maybe?

In general I like fast or no transitions.

In this case I suspect it goes a little bit deeper, so I will defer to @aduth and @mtias on some of the details — I believe the delay is there to ensure this sibling inserter doesn't pop up visually when you just wave the mouse across the blocks. I understand the issue this brings when you tab into it. I'm not sure what a great solution is.

Contributor

jasmussen commented Nov 23, 2017

This doesn't happen when hovering, only when focusing the button. @jasmussen is the transition really necessary? Or maybe make it a bit faster, maybe?

In general I like fast or no transitions.

In this case I suspect it goes a little bit deeper, so I will defer to @aduth and @mtias on some of the details — I believe the delay is there to ensure this sibling inserter doesn't pop up visually when you just wave the mouse across the blocks. I understand the issue this brings when you tab into it. I'm not sure what a great solution is.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Nov 23, 2017

Contributor

Yep I understand the delay is there as sort of hoverintent. If I remember correctly, when hovering also the tooltip is delayed. WHen tabbing, it's not so the difference in the "appearing time" is noticeable.

Contributor

afercia commented Nov 23, 2017

Yep I understand the delay is there as sort of hoverintent. If I remember correctly, when hovering also the tooltip is delayed. WHen tabbing, it's not so the difference in the "appearing time" is noticeable.

@aduth aduth deleted the fix/3408-ie11-focus branch Nov 27, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 27, 2017

Member

Noting that the tab delay existed previously. Might be able to drop the transition delay for focus specifically.

Member

aduth commented Nov 27, 2017

Noting that the tab delay existed previously. Might be able to drop the transition delay for focus specifically.

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