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

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

Closed
MoreauJonas opened this issue Sep 26, 2023 · 15 comments
Closed

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

MoreauJonas opened this issue Sep 26, 2023 · 15 comments
Labels

Comments

@MoreauJonas
Copy link

MoreauJonas commented Sep 26, 2023

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

@MoreauJonas MoreauJonas changed the title I found a bug: ... I found a bug: Tpography/text-decoration/none Sep 26, 2023
@MoreauJonas MoreauJonas changed the title I found a bug: Tpography/text-decoration/none I found a bug: Tpography/text-decoration:none Sep 26, 2023
@lexoyo lexoyo added the v3 label Sep 27, 2023
@lexoyo
Copy link
Member

lexoyo commented Mar 8, 2024

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
Copy link
Contributor

oliviermgx commented Mar 10, 2024

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
Copy link
Member

lexoyo commented Mar 10, 2024

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
#1497

@oliviermgx
Copy link
Contributor

oliviermgx commented Mar 13, 2024

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
Copy link
Member

lexoyo commented Mar 14, 2024

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
Copy link
Member

lexoyo commented Mar 14, 2024

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
Copy link
Contributor

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

@oliviermgx
Copy link
Contributor

oliviermgx commented Mar 14, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@lexoyo
Copy link
Member

lexoyo commented Mar 18, 2024

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
Copy link
Contributor

oliviermgx commented Mar 20, 2024

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
Copy link
Member

lexoyo commented Mar 21, 2024

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
Copy link
Contributor

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

@lexoyo
Copy link
Member

lexoyo commented Apr 3, 2024

Thanks @oliviermgx for the fix

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

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

No branches or pull requests

3 participants