silexlabs / Silex

Silex live web creation, free/libre no-code website builder, next gen Webflow for the static web
http://www.silex.me
GNU Affero General Public License v3.0
2.2k stars 580 forks source link

I found a bug: Tpography/text-decoration:none #1543

Closed MoreauJonas closed 4 months ago

MoreauJonas commented 11 months ago

Describe the bug Quand nous utilisons un nous ne pouvons enlever la text-decoration:underline En effet, le text-décoration dans "typographie" est déja sur "none" et nous ne pouvons pas le sélectionner

To Reproduce Steps to reproduce the behavior:

  1. Go to 'open style manager'
  2. Click on 'Typography'
  3. select "text-decoration"

Expected behavior Pouvoir enlever le "underline" des balises par défaut, le text-decoration est sur none, cependant, le none se s'applique pas image

If you use Silex desktop app, please complete the following information If you use Silex online, please complete the following information

URL: [e.g. editor.silex.me]

Browser [e.g. chrome]

Silex desktop version: 3.0.0-alpha.117

OS: [e.g. Windows 11]

Browser Chrome

lexoyo commented 5 months ago

this is also true for many the css properties

sometimes the default values are selected in a drop down but it should be a blank value by default instead. for example in the image: display, visibility, font family, white space, ...

Screenshot from 2024-03-08 14-53-06

And another similar problem for the "buttons" like properties float, position, text align, word wrap...

oliviermgx commented 5 months ago

I don't think these behaviors are linked or similar ? Concerning the text-decoration one, it seems that "initial" value cause pb in html when using compact declaration : text-decoration : none solid initial auto doesn't work in firefox, nor opera. But separated declaration : text-decoration-line: none; text-decoration-style: solid; text-decoration-color: initial; text-decoration-thickness: auto; should work. In Silex/src/ts/client/grapesjs/css-props.ts default value are given separatly (line, style, ...) but at the end html passed is in compact format.

Perhaps should we pass values in separated format, but I don't know how to do that up to now. An other way could be to pass : text-decoration: initial, it works in Firefox, but it gives value 'initial' to all properties ?

lexoyo commented 5 months ago

Oh ok If you open silex in firefox and save, it gives separate values then i guess To better parse the values we need to do this issue https://github.com/silexlabs/Silex/issues/1497

oliviermgx commented 5 months ago

Following #1497 I obtain image it seems that the "text-decoration : none solid initial auto" pb is not linked to this GrapesJS pb as it doesn't work in this test nor. Using property "currentcolor" instead of "initial" in src/ts/client/grapesjs/css-props.ts line 433 could solve the text-decoration-line undesired behavior: { name: 'Text decoration color', property: 'text-decoration-color', type: 'color', defaults: 'currentcolor', fixedValues: ['inherit', 'initial', 'revert', 'unset'], info: 'The text-decoration-color CSS property sets the color of decorations added to text by text-decoration-line.', } So, we can select and change text-decoration-line. bug detected by @MoreauJonas
But this doesn't solve the default value of "none" when defining a link for the first time. Ask me for a PR ?

lexoyo commented 5 months ago

Following #1493 I obtain

  1. Are you sure it's the issue you meant to link here?
  2. Did you try using different browsers?
lexoyo commented 5 months ago

Using property "currentcolor" instead of "initial" in src/ts/client/grapesjs/css-props.ts line 433 could solve the text-decoration-line undesired behavior

What about using '' (Empty string) to suppress the value alltogether ?

I hope i understand what you are saying

oliviermgx commented 5 months ago

Sorry, of course I would mean #1497 ! I will try with some browsers I will test empty string too thanks

oliviermgx commented 5 months ago

Setting defaults color to "currentcolor" in src/ts/client/grapesjs/css-props.ts line 433 makes possible setting the other text-decoration properties. Tested with Firefox 123.0, Chrome 122.0.6261.128 and Opera 108.0.5067.29 Setting same to "" (empty property) works fine on these 3 browsers. Setting to "initial" doesn't allow to set others properties (except if we select another color first).

oliviermgx commented 5 months ago

There are more than this problem with the bug detected by Jonas : The "none" property for text-decoration-line is not applied because no css rules are set in a just created Link element or no rules are loaded in the frame at the creation step. I don't know how to search why for the moment. I suggest to initiate the text-decoration-line to underline as a provisory workaround solution. This could be done by setting default text-decoration line to "underline" in src/ts/client/grapesjs/css-props.ts line 396. This doesn't solve the pb but could masks it as a provisory solution.

oliviermgx commented 5 months ago

By these facts, I am really sure that the bug detected by Jonas is not linked to the #1497 issue

lexoyo commented 5 months ago

suggest to initiate the text-decoration-line to underline as a provisory workaround solution. This could be done by setting default text-decoration line to "underline" in src/ts/client/grapesjs/css-props.ts line 396. This doesn't solve the pb but could masks it as a provisory solution.

What about setting it to empty string too?

oliviermgx commented 5 months ago

should we have all these properties blank in an init state ? image that is : Display ? Visibility Float Position Top, Right, etc ? Content (if pseudo exists) ?

lexoyo commented 5 months ago

should we have all these properties blank in an init state ?

I believe so because that is the true reflection of the css rule generated (no prop, no value)

For float and position I don't know what it will look like? Will there be no selection at all? (That would be perfect)

oliviermgx commented 5 months ago

For information, the demo page of grapesjs has the same default values https://grapesjs.com/demo.html image

lexoyo commented 4 months ago

Thanks @oliviermgx for the fix

When it will be in production we can ask @MoreauJonas for feedback