Skip to content
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

Update the authorship module from 0.20.1 to 1.0.0 #1049

Closed
wants to merge 1 commit into from
Closed

Update the authorship module from 0.20.1 to 1.0.0 #1049

wants to merge 1 commit into from

Conversation

benbro
Copy link
Contributor

@benbro benbro commented Oct 13, 2016

Test with:

 var quill = new Quill('#editor-container', {
    placeholder: 'Compose an epic...',
    theme: 'snow',
    modules: {
      authorship: {
        enabled: true,
        authorId: 'me',
        color: 'red'
      }
    }
  });

@sferoze
Copy link
Contributor

sferoze commented Oct 16, 2016

@benbro I still believe there is an issue with this module.

The cursor disappears as soon as you start typing. I set the background to transparent and still the cursor disappears. So this is not due to the cursor blending in with background.

    authorship:
          enabled: true
          authorId: 'me'
          color: 'transparent'

I am positive it is the authorship module, because when I comment out the authorship module when initializing quill, the cursor does not disappear. The cursor disappears yet the editor remains focused. If you press the back arrow key, the cursor reappears again. When you start typing, it disappears.

This is not an issue for you? Can you test again? When quill is updating the content silently maybe the cursor disappears.

@sferoze
Copy link
Contributor

sferoze commented Oct 16, 2016

Solution: #1057 (comment)

@jhchen
Copy link
Member

jhchen commented Oct 17, 2016

I'd like to see if we can not inject <style> tags. CSP strict rules are being pushed and Quill triggering these warnings will cause casual observers to panic.

@benbro
Copy link
Contributor Author

benbro commented Oct 17, 2016

I wasn't aware of CSP. How do you check what compatible with CSP directive of style-src: self?

What about using a style attributor instead of a class attributor?
This stackoverflow comment says that modifying the element style attribute is fine. I don't understand how it is different than any other change to CSS.
http://stackoverflow.com/questions/24713440/banned-inline-style-csp-and-dynamic-positioning-of-html-elements/29089970#29089970

Do you want this module in core?

@@ -28,6 +28,7 @@ import CodeBlock, { Code as InlineCode } from './formats/code';
import Formula from './modules/formula';
import Syntax from './modules/syntax';
import Toolbar from './modules/toolbar';
import Authorship from './modules/authorship';
Copy link
Contributor

Choose a reason for hiding this comment

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

The other modules are imported in a alphabetical order; so this should be at the top.

@@ -89,6 +90,7 @@ Quill.register({
'modules/formula': Formula,
'modules/syntax': Syntax,
'modules/toolbar': Toolbar,
'modules/authorship': Authorship,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@sferoze sferoze mentioned this pull request Nov 27, 2016
@sbevels
Copy link

sbevels commented Dec 14, 2016

Since the authorship module is unavailable in Quill, I tried adding this code as a custom module. I am having trouble getting quill.updateContents to work: this.quill.updateContents(authorDelta, Quill.sources.SILENT);

It returns nothing instead of what was in authorDelta and I don't see the author attribute when I call getContents to view the data. If I remove the author attribute from the delta, then updateContents works just fine. Is there something missing to register author as an attribute? I'm not sure how to get quill to accept a custom attribute.

@sferoze
Copy link
Contributor

sferoze commented Dec 14, 2016

@sbevels

The module needs to be built with Quill. I don't think you can add as custom module.

@danielschwartz
Copy link

danielschwartz commented Dec 19, 2016

@benbro just a heads up, this version of the Authorship causes a weird document state when applying/unapplying formatting (tested on Quill@v1.1.7):

Given a line that looks like this:

<p>
  <span class="ql-author-1">See 2015 Q4 Goals</span>
</p>

Make "See" bold so that the line now looks like this:

<p>
  <b class="ql-author-1">See</b>
  <span class="ql-author-1"> 2015 Q4 Goals</span>
</p>

Then unbold "See" and notice that the line now looks like the following instead of reverting to the initial example:

<p>
  <span class="ql-author-1">See</span> 2015 Q4 Goals
</p>

@benbro benbro mentioned this pull request Dec 20, 2016
@sferoze
Copy link
Contributor

sferoze commented Jan 8, 2017

@benbro @jhchen

Using quill 1.1.9 I found a new issue with the authorship module. I also have a hack (code below) which serves as a solution to this issue.

Say 2 different users are editing a quill document. Each user has a different authorClass.

When 2 different authors edit content you get dom html that looks like this:

<span class="ql-author-guest">Hello from Guest Author</span>
<span class="ql-author-ph3MwMWaGGMbP2qdR">Main Author is typing here</span>

Now imagine that I am author ql-author-ph3MwMWaGGMbP2qdR and I click at the end of the <span class="ql-author-guest"> created by the guest author to add a few words to the sentence Hello from Guest Author. When I start typing the next character goes in front of the cursor and the cursor gets stuck in the previous author span element.

I think this might be due to a browser bug? But now sure....

I was able to come up with a fix

  sel = rangy.getSelection()
  node = sel?.nativeSelection.anchorNode

  if sel.isCollapsed
    if node? and !$(node).hasClass('ql-clipboard')
      if Meteor.userId()
        authorClass = 'ql-author-' + Meteor.userId()
      else
        authorClass = 'ql-author-guest'

      if !$(node).hasClass(authorClass) and $(node).is('span') and ($(node).attr('class').indexOf('ql-author') isnt -1)
        if sel.rangeCount and sel.anchorOffset > 0
          if node.nextSibling? and $(node.nextSibling).hasClass(authorClass)
            range = sel.getRangeAt(0)
            range.collapse false
            
            range.collapseToPoint(node.nextSibling, 1)
            sel.setSingleRange range


            # The code above explains the issue and I should only need the code above. 
            # But for some reason I have to add the code below because otherwise 
            # the cursor gets placed at the end of the next span element instead of the beggining. 
            # Even though I specify the beggining at index 1.
            sel = rangy.getSelection()
            node = sel?.nativeSelection.anchorNode
            range.refresh()
            range.collapseToPoint(node, 1)
            sel.setSingleRange range

As you can see I am checking if the node you are typing in has your own authorClass. If not, that's an issue, and we need to move the cursor over to the next node at index 1. This is the newly created span element with your authorClass and the cursor is positioned right after the first character you started to type. From that point on everything is good because you are typing in the correct span element of your own authorClass.

The hack kicks in only when it detects that you are not typing in a span element of your own authorClass, which is the issue.

Tested with Quill 1.1.9 and all seems to be working with this fix.

Let me know if I should open a new issue for this and link it back to this one.

@sbevels
Copy link

sbevels commented Jan 9, 2017

@sferoze @benbro @jhchen

I believe the issue has to do with a fix made in the 1.1.7 release. Try testing with Quill 1.1.6 to see if this issue still exists. See #1152 for some more info on what change caused it.

@sferoze
Copy link
Contributor

sferoze commented Jan 10, 2017

@sbevels I am not understanding why this issue or change #1152 has affected the authorship module in the way I described.

@sferoze
Copy link
Contributor

sferoze commented Jan 15, 2017

Another issue I notice with the current authorship module has to do with the selection. This seems to be related to #1152 which @sbevels mentioned.

When you use the toolbar to format a word in the middle of a sentence, for example making a word bold. And then while the toolbar is still open, un-bold the word, the selection highlights the entire div.

You can try it here, I am running quill 1.1.9 with the authorship module.

www.memrey.com/n/test-quill

I am not sure what is going on or how to fix this. Any advice @sbevels ?

@sferoze
Copy link
Contributor

sferoze commented Jan 15, 2017

actually @sbevels I was able to fix it by referencing the #1152

I made the change on line 397 seen here 51c03ba

I changed

if (range != null && source === Emitter.sources.USER) {

to

if (range != null) {

And now the selection is preserved.

@sferoze
Copy link
Contributor

sferoze commented Jan 15, 2017

@sbevels And also after making the changes I saw in that commit you referred to the original hack is also not needed anymore! Thanks

@sferoze
Copy link
Contributor

sferoze commented Jan 15, 2017

@jhchen This commit 51c03ba

fixed the bugs with the authorship module I mentioned above. With the fixes in that commit the authorship module seems to be working perfectly fine now with no hacks needed.

@sferoze
Copy link
Contributor

sferoze commented Mar 20, 2017

@benbro @jhchen

An issue that needs to be fixed with authorship before official integration is to account for authorship in the history module.

Right now when you undo and redo edits with the history module, it does not account for authorship module class tags. Like if you undo 20 steps and the redo 20 steps, Quill will render all the edits under your authors username, instead of redoing the actual author that originally made the edit. Basically the history module needs to save authorship classes as well.

I have moved this issue over to a new issue thread.

#1371

@quill-bot
Copy link

Quill 2.0 has been released (announcement post) with many changes and fixes. If this is still an issue please create a new issue after reviewing our updated Contributing guide 🙏

@quill-bot quill-bot closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants