search

LadybirdBrowser / ladybird

Truly independent web browser
https://ladybird.org
BSD 2-Clause "Simplified" License
22.27k stars 987 forks source link

LibWeb: Allow custom properties in CSSStyleDeclaration.setProperty() #2460

Closed kalenikaliaksandr closed 1 day ago

kalenikaliaksandr commented 2 days ago

This change fixes unhoverable toolbar on https://excalidraw.com/ The problem was that React.js uses setProperty() to add style properties specified in the "style" attribute in the virtual DOM, and we were failing to add the CSS variable used to set the "pointer-events" value to "all".

Psychpsyo commented 2 days ago

Is this fixing the same thing as #1978 ?

(Also, this'll fix most of the styling on one of my pages that I've been trying to slowly make work as well, so :D )

kalenikaliaksandr commented 2 days ago

Is this fixing the same thing as https://github.com/LadybirdBrowser/ladybird/pull/1978 ?

yes, I overlooked this PR. I don't like that this PR makes set_property() accept both property_id and property_name even though property_id is inferred from property_name. I think it's bad API design.

@AtkinsSJ would love to hear your thoughts!

AtkinsSJ commented 1 day ago

Is this fixing the same thing as #1978 ?

yes, I overlooked this PR. I don't like that this PR makes set_property() accept both property_id and property_name even though property_id is inferred from property_name. I think it's bad API design.

@AtkinsSJ would love to hear your thoughts!

I think that's partly my fault. I was hoping to avoid the situation where we call set_property(PropertyID) and it converts that to a string, only to convert it back to a PropertyID again. I thought eventually it could be a struct containing both as Optionals, but it's definitely messier than just passing strings around.

kalenikaliaksandr commented 1 day ago

Is this fixing the same thing as #1978 ?

yes, I overlooked this PR. I don't like that this PR makes set_property() accept both property_id and property_name even though property_id is inferred from property_name. I think it's bad API design.

@AtkinsSJ would love to hear your thoughts!

I think that's partly my fault. I was hoping to avoid the situation where we call set_property(PropertyID) and it converts that to a string, only to convert it back to a PropertyID again. I thought eventually it could be a struct containing both as Optionals, but it's definitely messier than just passing strings around.

Agreed, converting back and forth between PropertyID and string is annoying, but having a function that accepts PropertyID is useful to have a type checking for usages within LibWeb.

AtkinsSJ commented 1 day ago

Yeah I wouldn't want to get rid of set_property(PropertyID) etc.