ueberdosis / tiptap

The headless rich text editor framework for web artisans.
https://tiptap.dev
MIT License
27.52k stars 2.29k forks source link

Text color seems to be automatically converted to RGB #1818

Open sereneinserenade opened 3 years ago

sereneinserenade commented 3 years ago

Description It seems like the colors that should be in hex, are automatically transformed to rgb and therefore the check for the color is failing.

https://user-images.githubusercontent.com/45892659/131475139-2cbc0921-af20-40ca-9acc-75f48340f178.mov

image

image

Steps to reproduce the bug Steps to reproduce the behavior:

  1. Go to https://www.tiptap.dev/api/extensions/color
  2. Set color for some text to any color you like
  3. See the color input in the menubar and you will notice that the color from the text that you changed will get recognized by input, but the color which was already there, i.e. purple(#958DF1) gets converted to RGB

Expected behavior The color shouldn't be converted to rgb so that we have consistent behaviour

Environment?

hanspagel commented 3 years ago

I think that’s a browser thing, but I’ll check it.

razh commented 3 years ago

Yep, it's a browser thing!

The computed value is the corresponding rgb() or rgba() value:

https://developer.mozilla.org/en-US/docs/Web/CSS/color#formal_definition

const div = document.createElement('div');
div.style.color = '#958DF1';
console.log(div.style.color);
// rgb(149, 141, 241)
hanspagel commented 3 years ago

Three things we could do:

  1. Recommend to use rgb
  2. Use rgb in the example
  3. Make isActive smart enough to convert it and check for both

I think we go with 1 and 2, not sure about 3.

Anyway, thanks for reporting!

philippkuehn commented 3 years ago

I fiddled around a bit. A funny side node: the native HTML color picker only works with hex colors 🙃

There are some options now:

  1. we only support HEX or RGB and convert between them to a specific format (defaults to HEX I think). It’s easy to convert between these two formats to this could happen within the color extension package.
  2. we support any color input (like HSL, LAB, ...) and convert it to a specific format. This requires an external library.
  3. we prevent any color conversion (so that hex colors are not converted to RGB for example). So everything is up to the user. This may require the user to convert colors somehow before/after.

What do you think?

hanspagel commented 3 years ago

+1 for 1

we only support HEX or RGB and convert between them to a specific format (defaults to HEX I think). It’s easy to convert between these two formats to this could happen within the color extension package.

sereneinserenade commented 3 years ago

+1 for 1. same as @hanspagel , also would love to contribute

domnantas commented 3 years ago

What about cases where alpha channel is set (rgba() and #rrggbbaa)? Chrome seems to convert #rrggbbaa to rgba():

const div = document.createElement('div');
div.style.color = '#958DF100';
console.log(div.style.color);
// rgba(149, 141, 241, 0)

I also agree with the 1 option – it would cover most use cases out of the box and if the user wants to use a different color space, they would simply need to convert color to RGB or HEX.

philippkuehn commented 3 years ago

yeah with HEX/RGB I meant HEX/RGB/RGBA :)

lets’s do option 1 then!

philippkuehn commented 3 years ago

Seems like that not every value will be converted by the browser :(

const div = document.createElement('div');
div.style.color = '#958DF100';
console.log(div.style.color);
// rgba(149, 141, 241, 0)
const div = document.createElement('div');
div.style.color = 'red';
console.log(div.style.color);
// red

So option 1 is not enough I think.

philippkuehn commented 3 years ago

Ok some more thoughts.

color2k seems like a great small library and good fit for our needs.

But converting always to Hex will cause other issues.

  1. isActive will fail for other color formats than Hex Should we add an extension option convertToHex to opt-out?

    editor.commands.setColor('rgb(0,0,0)')
    editor.isActive('textStyle', { color: 'rgb(0,0,0)' }) // false -> would expect this returns true
    editor.isActive('textStyle', { color: '#000000' }) // true
  2. what about the highlight extension? There is a multicolor option which allows to use any color. Should we convert them to Hex too?

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

domnantas commented 2 years ago

Issue still relevant

domnantas commented 2 years ago

Please reopen this, this issue is still relevant

thieleju commented 2 years ago

@philippkuehn Sorry for tagging :/ Please reopen this issue, I'm currently struggling with this

edit: My workaround for now

function convertRgbColorsToHex(string) {
  return string.replace(/rgb\(\d+,\s*\d+,\s*\d+\)/g,
    (rgbString) => "#" + rgbString
      .match(/\b(\d+)\b/g)
      .map((digit) =>
        parseInt(digit).toString(16).padStart(2, "0").toUpperCase()
       )
      .join("")
    )
}
mattvb91 commented 2 years ago

Currently having the same issue, @thieleju kind of fixes it in converting it but still seeing the error happening.

drezr commented 1 year ago

Same issue here.

Jhoydev commented 1 year ago

Same issue here.

bdbch commented 1 year ago

I put it back into our 2.0.0 Release Milestone.

JahnoelRondon commented 1 year ago

I just ran into this same issue. This issue also occurs when you are setting content on update for the editor. all of your styles will get parsed into rgb for hex styles. Hope the next update fixes this,

https://github.com/ueberdosis/tiptap/issues/4167

tsl1127 commented 1 year ago

Same issue here.

eastecho commented 1 year ago

Made an extension to it, also added support for backgroundColor, hope this will help:

import Color from '@tiptap/extension-color'

declare module '@tiptap/core' {
  interface Commands<ReturnType> {
    backgroundColor: {
      setBgColor: (backgroundColor: string) => ReturnType,
      unsetBgColor: () => ReturnType,
    }
  }
}

// rgbStyleToHex
// from https://stackoverflow.com/a/3627747/5433572
// and https://stackoverflow.com/questions/5623838/rgb-to-hex-and-hex-to-rgb
function rgbStyleToHex(color: string | null): string | null {
  if (!color || color.indexOf("rgb") < 0) {
    return color
  }

  if (color.charAt(0) == "#") {
    return color
  }

  const nums = /(.*?)rgb\((\d+),\s*(\d+),\s*(\d+)\)/i.exec(color),
    r = parseInt(nums![2], 10).toString(16),
    g = parseInt(nums![3], 10).toString(16),
    b = parseInt(nums![4], 10).toString(16)

  return ("#"+ ((r.length == 1 ? "0"+ r : r) + (g.length == 1 ? "0"+ g : g) + (b.length == 1 ? "0"+ b : b))).toUpperCase()
}

export const MyColor = Color.extend({
  addGlobalAttributes() {
    return [
      {
        types: this.options.types,
        attributes: {
          color: {
            default: null,
            parseHTML: element => element.style.color?.replace(/['"]+/g, ''),
            renderHTML: attributes => {
              if (!attributes.color) {
                return {}
              }

              return {
                style: `color: ${rgbStyleToHex(attributes.color)}`,
              }
            },
          },
          backgroundColor: {
            default: null,
            parseHTML: element => element.style.backgroundColor?.replace(/['"]+/g, ''),
            renderHTML: attributes => {
              if (!attributes.backgroundColor) {
                return {}
              }

              return {
                style: `background-color: ${rgbStyleToHex(attributes.backgroundColor)}`,
              }
            },
          },
        },
      },
    ]
  },

  addCommands() {
    return {
      ...this.parent?.(),
      setBgColor: backgroundColor => ({ chain }) => {
        return chain().setMark('textStyle', { backgroundColor }).run()
      },
      unsetBgColor: () => ({ chain }) => {
        return chain().setMark('textStyle', { backgroundColor: null }).removeEmptyTextStyle().run()
      },
    }
  },

})
Ehsan200 commented 11 months ago

Made an extension to it, also added support for backgroundColor, hope this will help:

import Color from '@tiptap/extension-color'

declare module '@tiptap/core' {
  interface Commands<ReturnType> {
    backgroundColor: {
      setBgColor: (backgroundColor: string) => ReturnType,
      unsetBgColor: () => ReturnType,
    }
  }
}

// rgbStyleToHex
// from https://stackoverflow.com/a/3627747/5433572
// and https://stackoverflow.com/questions/5623838/rgb-to-hex-and-hex-to-rgb
function rgbStyleToHex(color: string | null): string | null {
  if (!color || color.indexOf("rgb") < 0) {
    return color
  }

  if (color.charAt(0) == "#") {
    return color
  }

  const nums = /(.*?)rgb\((\d+),\s*(\d+),\s*(\d+)\)/i.exec(color),
    r = parseInt(nums![2], 10).toString(16),
    g = parseInt(nums![3], 10).toString(16),
    b = parseInt(nums![4], 10).toString(16)

  return ("#"+ ((r.length == 1 ? "0"+ r : r) + (g.length == 1 ? "0"+ g : g) + (b.length == 1 ? "0"+ b : b))).toUpperCase()
}

export const MyColor = Color.extend({
  addGlobalAttributes() {
    return [
      {
        types: this.options.types,
        attributes: {
          color: {
            default: null,
            parseHTML: element => element.style.color?.replace(/['"]+/g, ''),
            renderHTML: attributes => {
              if (!attributes.color) {
                return {}
              }

              return {
                style: `color: ${rgbStyleToHex(attributes.color)}`,
              }
            },
          },
          backgroundColor: {
            default: null,
            parseHTML: element => element.style.backgroundColor?.replace(/['"]+/g, ''),
            renderHTML: attributes => {
              if (!attributes.backgroundColor) {
                return {}
              }

              return {
                style: `background-color: ${rgbStyleToHex(attributes.backgroundColor)}`,
              }
            },
          },
        },
      },
    ]
  },

  addCommands() {
    return {
      ...this.parent?.(),
      setBgColor: backgroundColor => ({ chain }) => {
        return chain().setMark('textStyle', { backgroundColor }).run()
      },
      unsetBgColor: () => ({ chain }) => {
        return chain().setMark('textStyle', { backgroundColor: null }).removeEmptyTextStyle().run()
      },
    }
  },

})

In parseHTML you must do this to make it work:

// for color:
rgbStyleToHex(element.style.color?.replace(/['"]+/g, ''))

// for backgroundColor
rgbStyleToHex(element.style.backgroundColor?.replace(/['"]+/g, ''))
EskelCz commented 4 months ago

That works, thanks!

SalahAdDin commented 2 months ago

So we need to parse it to Hex, s***t!

nperez0111 commented 2 months ago

I'd be happy to take a PR for this to be included as part of the color extension. Should probably warn on any other color values set

SalahAdDin commented 2 months ago

I just checked it, first it gets the color in rgba, and later it gets the color in hex, why?