Opened 9 years ago
Closed 9 years ago
#13421 closed Bug (fixed)
When pasted link cannot be autoembedded should there be an error notification?
Reported by: | Piotrek Koszuliński | Owned by: | Szymon Kupś |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.5.2 |
Component: | General | Version: | 4.5.0 |
Keywords: | Cc: |
Description
- Open http://tests.ckeditor.dev:1031/tests/plugins/autoembed/manual/autoembed
- Paste: http://foo.bar.com/
- After a while an error is shown. You need to close "x" to close it. It's irritating.
So:
- Do we want to keep the blue notification and drop the red one?
- Or do we want to remove all notifications?
Change History (14)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
But you know whether it's an autoembeddable link once it's... embedded. There should be no error notifications definitely - that's clear. The question is - when should there be info notifications after pasting links - always or never?
comment:3 Changed 9 years ago by
I agree that the current behavior is very wrong.
I can just predict that notifications are shown if an "embeddable" link is pasted but it doesn't get transformed into the embed. If this doesn't happen at the client side (by sniffing the URL), it *may* happen at server side and the server response is an error instead of the actual embed.
For all other cases ( successful embed and no embed), any notification should be displayed.
comment:4 Changed 9 years ago by
@fredck: Frankly speaking, I don't understand what you mean :D
To clarify one thing - on the client side we have pretty much no idea that a link will be embedded or not. There is a isUrlValid() check, but by default it only checks whether an URL is a valid URL. (BTW. I'll get to that later - we forgot to do this check now).
This means that right after a link is pasted (if it matches the criterium of being a "URL-only content" - I'll explain this criterium later on):
- we can display notification the default notification "Fetching oEmbed response...". But it will be displayed for all pasted links unless someone handles embed#validateUrl in a custom way.
- alternatively, we can display nothing at all while we wait for a response.
Then goes the second step - link is embedded or not.
- we agree here that if link was not embedded there should be no error notification. This is clear.
- I think that we also agree that a success notification should be displayed when a link was embedded. This is to notify user what has happened.
I forgot that we should call isUrlValid() which can synchronously do some basic validation. There's also validateUrl
event fired by this method, so a developer is able to implement a client-side synchronous validation. That's why we should also use it before auto embedding.
Second thing - I decided to go with a criterium that "URL-only content" is embedded. It means that if you paste a URL, then it's gonna be embedded. But if you paste a text linking to something, then auto embedding won't be executed. Examples:
http://foo.bar
- OK<a href="http://foo.bar">http://foo.bar</a>
- OKSee: http://foo.bar
- NOK- `<a href="http://foo.bar">pics of my dog</a> - NOK
In other words - we try to guess whether someone pasted a URL. Not a link or links or text with links.
comment:5 Changed 9 years ago by
Status: | new → confirmed |
---|
comment:6 Changed 9 years ago by
Owner: | set to Szymon Kupś |
---|---|
Status: | confirmed → assigned |
comment:7 Changed 9 years ago by
We need to choose one of the scenarios presented by Reinmar in comment:4.
What I understand, the main problem is to determine (on client side) whether the link can be embedded or not.
Since there is no synchronous way of detecting if link is "embeddable" I would go with skipping the notification during loading and display information only if embedding was finished correctly. However this solution can confuse users - embedding process might take a while and without any notification there is no way to say if anything is happening.
comment:8 Changed 9 years ago by
It all comes down to what I had said a long time ago before the Notification was implemented: it's not a generic solution for all sorts of problems. If we used inline notifications (like spinners or sth similar), there would be nothing irritating about a number of actions that failed because failures wouldn't be a huge notification stack that covers your content.
But since there's nothing we can do about it at the moment, let's suppress the damage:
- Always display "fetching oembed response..." notification, no matter what kind of link has been pasted.
- Users must know that in a moment a link they pasted may be transformed into something else.
- If the link gets transformed without prior information it looks like editor has some AI and does things spontaneously, on a whim.
- After the response has been fetched:
- Display blue notification if it failed.
- Blues will disappear so we avoid the "stack–of–hell that covers my content" effect.
- Blues mean no additional action for the user.
- Display no notification if succeeded. The widget is the proof of success.
- Display blue notification if it failed.
comment:9 Changed 9 years ago by
I have to agree with a.nowodzinski on this one. I would reconsider if "Fetching oembed response..." is the most user-friendly we can come up with.
It would be great if we have spinners next to links that are processed, though. Either as a global mechanism or for autoembed plugin purposes only. Maybe we could display them in little tooltips so they are slightly more visible?
comment:10 follow-up: 11 Changed 9 years ago by
Display blue notification if it failed.
Don't you feel that it should say something different than the red one which media embed plugin displays normally? Now it says:
Failed to fetch content for the given URL.
I think it should be:
This URL could not be embedded.
comment:11 Changed 9 years ago by
Replying to Reinmar:
Display blue notification if it failed.
Don't you feel that it should say something different than the red one which media embed plugin displays normally? Now it says:
Failed to fetch content for the given URL.
I think it should be:
This URL could not be embedded.
+1
comment:12 Changed 9 years ago by
Status: | assigned → review |
---|
Pushed branch:t/13421.
My solution is to disable notifications coming from embedbase
(only when auto embedding is performed). This way autoembed
plugin can show its own notifications:
- when auto embedding process is in progress,
- when there was an error during auto embedding.
Both notifications are created with type info
so they automatically hide after a while.
comment:13 Changed 9 years ago by
Status: | review → review_passed |
---|
comment:14 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Merged git:649f074 into master.
I'm for removing notifications, unless it's an auto–embeddable link. They bring nothing but confusion.