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

Fixed image add link input disappears issue #5942

Merged
merged 4 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@faishal
Contributor

faishal commented Apr 3, 2018

Fixes #5894
Same as #3822

Description

When you begin typing anything in the image link input, it simply just closes the UI due to key events propagation, changing the states and triggering updates.

How Has This Been Tested?

Add image block and try to add a link from the toolbar. When you start typing, input should stay open.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Apr 4, 2018

This fixes it for me, by the way.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Apr 10, 2018

Thanks for the pull request, @faishal

@WordPress/gutenberg-core Do you think it's possible to add e2e tests for this?

@aduth

This comment has been minimized.

Member

aduth commented Apr 10, 2018

just closes the UI due to key events propagation, changing the states and triggering updates.

Propagating to where to do what state changes?

Asking because while it occasionally has merit, I am very cautious of introducing Event#stopPropagation since in solving one issue it has a tendency to subtly introduce other, much more difficult to debug issues. If we need it, we should be clear at what point it fails, whether it's relevant for the component in question to have awareness of said propagation (e.g. a components/ component shouldn't be made to tailor to the editor specifically), and ultimately to document its existence very clearly for the next maintainer.

@youknowriad youknowriad added this to the 2.7 milestone Apr 18, 2018

#5894:Image add link input disappear when start typing, fixed it by s…
…topping event propagation

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 18, 2018

I rebased this but it looks like the suggestions are not showing up properly in the image link?

Fix issues with suggestions not working in image block
The issue was that on mobile, the block toolbar has overflow auto so as to scroll horizontally. This hid the suggestions box in a weird scrollbar.

This PR tweaks that, so the suggestions don't show up on mobile at all. This is not an ideal fix, but suggestions weren't very mobile friendly in the first place.

Suggest we get this in now as a bandaid, and then look at addressing suggestions on mobile separately, for example using the same technique the block switcher uses.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 18, 2018

Okay, I pushed a fix to make sure the link suggestions show up when making image links.

The issue was that on mobile, the block toolbar has overflow auto so as to scroll horizontally. This hid the suggestions box in a weird scrollbar.

I fixed so suggestions don't show up on mobile at all. This is not an ideal fix, but suggestions weren't very mobile friendly in the first place. Suggest we get this in now as a bandaid, and then look at addressing suggestions on mobile separately, for example using the same technique the block switcher uses.

Screenshots:

screen shot 2018-04-18 at 11 57 16

screen shot 2018-04-18 at 11 57 30

screen shot 2018-04-18 at 11 57 41

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 18, 2018

Also fixed an issue with link suggestions in captions:

screen shot 2018-04-18 at 12 32 10

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 18, 2018

I also updated the way we fix the event propagation from closing the toolbar. This was happening because the ObserveTyping component was observing typing in link modals as well. I updated the PR to avoid observing typing in block toolbars. The classname is hardcoded but I think it's fine since it's defined in the same module.

@youknowriad

LGTM 👍

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 18, 2018

Merging, this bug fix is important to get in before the release.

@youknowriad youknowriad merged commit 7e25ca2 into WordPress:master Apr 18, 2018

2 checks passed

codecov/project 43.84% (-0.01%) compared to 4116526
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@include break-small() {
margin: -1px;
}
}

This comment has been minimized.

@aduth

aduth Apr 18, 2018

Member

Files should end in newline.

Tip: Enforced automatically if installing an EditorConfig plugin.

@@ -120,7 +120,8 @@ class ObserveTyping extends Component {
// Abort early if already typing, or key press is incurred outside a
// text field (e.g. arrow-ing through toolbar buttons).
if ( isTyping || ! isTextField( target ) ) {
// Ignore typing in a block toolbar
if ( isTyping || ! isTextField( target ) || target.closest( '.editor-block-toolbar' ) ) {

This comment has been minimized.

@aduth

aduth Apr 18, 2018

Member

I understand this was merged because it's a bug, but we should be more explicit about when typing should and should not be triggered. It may well be that this is the correct approach to look for a toolbar ancestor, though could also be that it's a single fix to a larger problem. Are there other text fields in editor which shouldn't trigger isTyping ?

One such example on a quick glance is the inserter search for the default block, where the inserter button inexplicably disappears upon typing:

search

I originally liked that ObserveTyping was moved out of the block component because it's something of a global behavior, and BlockListBlock already has an abundant number of concerns, but am thinking more and more that it should at least be scoped in some way to the contents of a block. This still leaves open some possibility for issues with things like link inputs in RichText though.

This comment has been minimized.

@youknowriad

youknowriad Apr 18, 2018

Contributor

This still leaves open some possibility for issues with things like link inputs in RichText though.

I think triggering isTyping in that case is the correct behavior.

--
For me, All inputs and richtext in block edit functions should trigger the behavior. Side UI shouldn't. All side UI is triggered is shown outside the editor content area (Popovers) aside the toolbar, so the fix made sense.

But I understand that wrapping only the BlockEdit component with this might also fix the issue. It would have been a good approach too.

@aduth aduth referenced this pull request Apr 18, 2018

Merged

Add polish #6138

nuzzio added a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Fixed image add link input disappears issue (WordPress#5942)
* WordPress#5894:Image add link input disappear when start typing, fixed it by stopping event propagation

Signed-off-by: Faishal Saiyed <saiyedfaishal@gmail.com>

* Fix issues with suggestions not working in image block

The issue was that on mobile, the block toolbar has overflow auto so as to scroll horizontally. This hid the suggestions box in a weird scrollbar.

This PR tweaks that, so the suggestions don't show up on mobile at all. This is not an ideal fix, but suggestions weren't very mobile friendly in the first place.

Suggest we get this in now as a bandaid, and then look at addressing suggestions on mobile separately, for example using the same technique the block switcher uses.

* Fix issue with caption links

* Ignore typing in block toolbars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment