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

Link button broken #938

Closed
mpriscella opened this issue Sep 8, 2016 · 17 comments · Fixed by #944
Closed

Link button broken #938

mpriscella opened this issue Sep 8, 2016 · 17 comments · Fixed by #944

Comments

@mpriscella
Copy link

The link button on my toolbar is failing to link the selected text.

Steps for Reproduction

// Toolbar Options.
[
  [{ header: [false, 1, 2] }],
  ["bold", "italic", "underline"],
  ["blockquote"],
  ["link"]
]
  1. Create new Quill editor with the above toolbar options.
  2. Insert text and then select it.
  3. Click the 'link' toolbar button.

Expected behavior:
Highlighted text is converted into an anchor tag.

Actual behavior:
Highlighted text is unaffected and the following error appears in the console

Uncaught TypeError: Cannot read property 'getBoundingClientRect' of null.

If you refer to the js/quill.js file in the 1.0.3 release tarball, the error is happening around line 9420 (which seems to originate here).

Platforms:
Google Chrome 53.0.2785.101 (64-bit) running on OSX 10.11.6

Version:
1.0.3

@benbro
Copy link
Contributor

benbro commented Sep 8, 2016

@mpriscella I've created a codepen with your toolbar options.
Can you reproduce it here?
http://codepen.io/anon/pen/dpoLWq

I don't see an error with Chrome 53 on Windows 7.

@mpriscella
Copy link
Author

mpriscella commented Sep 8, 2016

@benbro Nope, seems to be working there.

Here's a screenshot of the behavior I'm experiencing, I'm still looking around the codebase but it looks like the tooltip is having trouble getting the bounds which is why it's off screen.

Are there any other toolbar modules that use a tooltip which I could test? Maybe I'll be able to get some more helpful information.

@benbro
Copy link
Contributor

benbro commented Sep 8, 2016

@jhchen might figure it from your screenshot.
Can you create a codepen that reproduce it?
Do you see the error on one of the demos on the website?

The video and formula(fx button) modules use a tooltip.

@mpriscella
Copy link
Author

Ahh, here we go -

http://codepen.io/mpriscella/pen/pEJBKZ

My container element doesn't have a defined height.

@benbro
Copy link
Contributor

benbro commented Sep 8, 2016

With your codepen the tooltip doesn't show but I don't see an error in the console.

@mpriscella
Copy link
Author

I don't see the error either. I tried downloading http://cdn.quilljs.com/1.0.3/quill.js and using that in my code but I received the same error. Does codepen display library errors in the console?

@jhchen
Copy link
Member

jhchen commented Sep 8, 2016

I'm not seeing any errors on Mac + Chrome (same version is reported). Codepen displays errors to the console. You can just create an error to confirm.

There is a bounds property to tell Quill where it can show up horizontally. By default it is the whole page and from the screenshot it looks like you have something with a higher z-index covering it. If there's not space to show the tooltip vertically, it is by default hidden. You can overwrite the CSS rule if you plan to have the editing container autogrow, but will cause problems when you scroll:

.ql-snow .ql-out-bottom {
  visibility: visible;
}

@abelousov
Copy link

To get the error, theme should be set to 'snow'; that theme relies on bounds option, so the fix could be to check for that option, and if absent, throw an error that explains how to set it correctly (in a perfect world it could link to corresponding docs section)

Can't do it myself, as I have no idea how should be bounds described correctly

@etherbob
Copy link

etherbob commented Sep 9, 2016

I had a similar problem while trying to use it with a rails app I'm working on. Had to add
Rails.application.config.assets.precompile += %w( quill.js ) to config/initializers/assets.rb and pull the script in inside my edit page (vs application.js) to get links working.

@jhchen
Copy link
Member

jhchen commented Sep 9, 2016

@abelousov bounds is never absent. It is document.body by default.

@mpriscella
Copy link
Author

@jhchen I tried again being sure to set a defined height in my css but it's still failing for me. I also looked for any elements with high z-index's per your comment and was able to verify that there were none on the page. I've also removed any unnecessary javascript files from the page in the off chance that they could affect this (they didn't).

As far as I can tell this.options.bounds is null when being passed to SnowToolTip in themes/snow.js at line 29, unless boundsContainer is modified somewhere else before being passed to ToolTip (defined in ui/tooltip.js).

@etherbob
Copy link

etherbob commented Sep 9, 2016

@jhchen found a way to reproduce it in chrome/firefox/safari on a mac, but not in codepen

In the 1.0.3 distribution download. Modify the full example to load quill.js in the header instead of the body.

@schneidmaster
Copy link

I've reproduced a similar problem. In my case, I am 1) using Webpack to bundle assets and 2) dynamically creating Quill's using jQuery on clicks. I don't really have time at the moment to dive into debugging but I suspect that something in those use cases causes document.body to be undefined when the defaults are set. In my case, I got around it just by manually setting the bounds option when creating the Quill instance.

@benbro
Copy link
Contributor

benbro commented Sep 9, 2016

Steps for Reproduction

  1. Load the html document below.
  2. Type "Test"
  3. Select the text and click on the link button.
    Error in the console:
TypeError: this.boundsContainer is null

As @etherbob said, when moving the quill.js script tag to the body, the error goes away.
That's because document.body isn't defined yet.

Quill.DEFAULTS = {
  bounds: document.body,
}

https://github.com/quilljs/quill/blob/b7c623507c16df63fa4466f4dffb1541824b115c/core/quill.js#L301

<!DOCTYPE html>
<html>
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
    <style>
      #editor-container {
        height: 375px;
      }
    </style>
    <link href="//cdn.quilljs.com/1.0.3/quill.snow.css" rel="stylesheet" type="text/css" />    
    <script src="//cdn.quilljs.com/1.0.3/quill.js"></script>
</head>
<body>
    <div id="editor-container"></div>
    <script>
      var quill = new Quill('#editor-container', {
        modules: {
          toolbar: [
            ['link']
          ]
        },
        theme: 'snow'
      });
    </script>
</body>
</html>

@benbro
Copy link
Contributor

benbro commented Sep 9, 2016

A possible fix:

class Tooltip {
  constructor(quill, boundsContainer) {
    this.boundsContainer = boundsContainer || document.body;

@mpriscella
Copy link
Author

mpriscella commented Sep 9, 2016

Ahh, that makes sense. Modified my toolbar options to the following and the link tooltip is now working as expected:

// Toolbar Options.
{
  modules: {
    toolbar: [
      [{ header: [false, 1, 2] }],
      ["bold", "italic", "underline"],
      ["blockquote"],
      ["link"]
    ]
  },
  bounds: document.body,
  theme: "snow"
}

Thanks for the insight!

@lEduFranco
Copy link

The way I found to adjust this tooltip using only css is this:

 .ql-snow .ql-tooltip {
    width: 50%;
    height: fit-content;
    transform: translateX(47%) translateY(10px);
  }

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

Successfully merging a pull request may close this issue.

7 participants