Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

Tests for createlink bugs #190

Merged
merged 8 commits into from
Jun 13, 2014
Merged

Conversation

samvasko
Copy link
Contributor

@samvasko samvasko commented Jun 9, 2014

Inserting link currently fails reliably on BROWSER_NAME='firefox' BROWSER_VERSION='29' PLATFORM='WINDOWS'

@samvasko
Copy link
Contributor Author

samvasko commented Jun 9, 2014

Also I am working on #189 so do not merge this one just yet.

@OliverJAsh
Copy link
Contributor

Ahh, so this occurs when there is no selection.

@@ -400,4 +400,21 @@ describe('commands', function () {
});
});
});

describe('createlink', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command name should be camelcased.

@samvasko
Copy link
Contributor Author

samvasko commented Jun 9, 2014

I just noticed this from MDN

createLink
Creates an anchor link from the selection, only if there is a selection...

It looks like this is not actually a bug from their perspective.

@OliverJAsh
Copy link
Contributor

Interesting. This is the trouble – there is no spec for commands!

I’d love to get this added to the list of browser inconsistencies with an isolated edge case. Would you mind doing that?

One option would be to disable the command if there is no selection, however then you lose the feature Chrome has…

@cojennin
Copy link

cojennin commented Jun 9, 2014

What's the expected behavior of this plugin?
(On Ubuntu with Chrome 35, Firefox 29 and Nightly).

  • On Chrome, selection.collapsed = true: A link element is created and inserted at the the position of the caret. Example Hello, Wo<a href="http://this.is.a.test">this.is.a.test</a>rld!
  • On Chrome, selection.collapsed = false: A link element is created, the text of which is the selection. Example: Hello, <a href="http://this.is.a.test">World!</a>
  • On Firefox (29, Nightly), selection.collapsed = true: Nothing is created, or inserted. Firefox does nothing.
  • On Firefox (29, Nightly), selection.collapsed = false: A link element is created, the text of which is the selection. Example: Hello, <a href="http://this.is.a.test">World!</a>

Not sure what the best approach to be. Looking around

  • TinyMCE 4.0 (the demo on their homepage) seems to prefer Chrome's behavior and implements it in Firefox (I imagine with some minor heuristics, maybe avoiding createLink altogether?)
  • TinyMCE 4.0 as implemented by WordPress (looking at 3.9.1) prefers "when selection.collapse = true, disable the Link button on both Chrome and FF"

I'm not sure what the preferred use case should be. I'd like to ask around and see if folks are used to one or the other. (Honestly, my preference would be "if my caret is in a word and selection.collapsed = true, when the Link button is selected the whole word should become a link" but from the looks of it that might be prone to inconsistencies across browsers. So my runner up would be "Link button is disabled when selection.collapsed = true." Avoids the heuristics, better consistency + I think most people already have some text in mind they want to make into a link or they're willing to type it out real quick and then link it).

@samvasko
Copy link
Contributor Author

samvasko commented Jun 9, 2014

What's the expected behavior of this plugin

I think a plugin (UX/UI) implementation of this should not be discussed here. Lets just concentrate on behavior of createLink in context of scribe should be as consistent as possible.

Maybe we could check if selection is collapsed and if it is.

  1. Insert text node
  2. Wrap it in selection
  3. do a createLink

But that seems rather complex. I would definitely prefer to keep the chrome behavior. Not sure what is the best path to get it.

@cojennin
Copy link

cojennin commented Jun 9, 2014

Sorry, wasn't trying to imply a UX/UI discussion. Was merely noting that the implementation of createLink is inconsistent across browsers (FF requires a selection, Chrome does not), thus the behavior of createLink in Scribe needs to be established first. Since the Link plugin seems to be where createLink is utilized most extensively, a good place to start for establishing this behavior might be there.

I get the idea of choosing the Chrome behavior, but then should createLink be re-implemented in Firefox (to support the same behavior? Since by default, FF requires a selection to run createLink)? And if so, does that mean any plugin that wants to utilize createLink should be required to do it in a uniform way, or should that be left up to the plugin? (as in, should all plugins use a "scribeCreateLink" so that they provide consistency across browsers, or should that be something to opt into?)

@samvasko
Copy link
Contributor Author

@OliverJAsh Is there something stopping us from just using insertHTML? I may be overlooking something...

@cojennin
Copy link

I don't have a lot of background in this so that sounds like it'd make a lot of sense. Would running something like execCommand('unlink') still work as expected?

@cojennin
Copy link

Looking at the browser inconsistency documentation, seems like it might come with some issues of its own. But still sounds like it could bring better consistency than createLink. I'll try and mess around with it a bit.

@samvasko samvasko changed the title Failing test for createlink bugs Tests for createlink bugs Jun 11, 2014
@samvasko
Copy link
Contributor Author

You have a good point. If there will be usage for createLink it will probably be used thru that plugin, and the current behavior in Firefox is quite counter-intuitive. I think best way would be to create patch for createLink that checks if selection is collapsed and does insertHTML instead.

Would you like doing that? @cojennin

@OliverJAsh
Copy link
Contributor

Re. #190 (comment), I recommend reading how we commands are architected in Scribe here: https://github.com/guardian/scribe#commands.

Similarly, we’ve patched many other browser inconsistencies: https://github.com/guardian/scribe/tree/master/src/plugins/core/patches/commands

The command would still be called createLink. You would get an instance of it with scribe.getCommand('createLink').

You can wrap your patch as a plugin for easy distribution. See the core patches (link above) for an example.

@OliverJAsh
Copy link
Contributor

You don't need to use insertHTML to write your patch. You could just do direct DOM manipulation.

@samvasko
Copy link
Contributor Author

OK, I did the patch and tests seem to be passing. I checked with blink source and there is only 4 lines for this. So I am pretty sure there is no extra magic going on behind the scenes.

I also slightly modified the behavior of the selection. Now instead of selecting the link it just places the caret after it.

// Place caret at the end of link
var newRange = document.createRange();
newRange.setStartAfter(aElement);
newRange.setEndAfter(aElement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, do you need to set both the start and end if the selection is collapsed? I’m never sure, but it would be good to familiarise myself with the API more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works both ways for me on Chrome/FF. I would guess undefined behavior as usual. 😫

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks as if range.setStartAfter also sets a matching end container and offset. This isn't documented. The only spec for Range (2000) I could find is here, which doesn't specify what should happen in this instance: http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html

If it’s something that just happens to be implemented in both Chrome and Firefox, we should probably stick with the spec and use setStartAfter and setEndAfter.

scribe.api.CommandPatch.prototype.execute.call(this, value);

// Place caret at the end of link
selection.selection.collapseToEnd();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like it’s part of the same patch. What was your reasoning for this? Is this another browser inconsistency you're patching?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned it in above comment. I pretty much by accident did it for the patched one. Then I did some scouting (Tiny MCE, Gmail, Google Docs, Medium) and I figured that this is more logical behavior. But that is of course matter of debate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that’s what the big applications do, I think it would be good for us to do it!

This sort of thing isn't a patch, so I think it belongs as a Scribe command instead of a Scribe command patch (this is just an infrastructure we have for separating concerns in our code). Here are some examples of Scribe commands: https://github.com/guardian/scribe/tree/master/src/plugins/core/commands

Your command patch’s execute method will be invoked by your command’s execute method.

execute: function (value) {
  scribe.api.Command.prototype.execute.call(this, value);

  selection.selection.collapseToEnd();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when I add command into core/commands it will be ignored by other higher lever commands like link prompt command. Because they declare their own commands!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OliverJAsh
Copy link
Contributor

This is good to go! If you want to implement the other changes proposed, please raise another PR.

Thanks :-)

OliverJAsh pushed a commit that referenced this pull request Jun 13, 2014
@OliverJAsh OliverJAsh merged commit 7f2fcd8 into guardian:master Jun 13, 2014
@samvasko
Copy link
Contributor Author

Awesome :)

@OliverJAsh
Copy link
Contributor

Available in v0.1.11.

@samvasko samvasko deleted the createLink-bugs branch July 26, 2014 09:31
@samvasko samvasko restored the createLink-bugs branch July 26, 2014 09:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants