Dyalog / ride

Cross-platform IDE for Dyalog APL
https://dyalog.github.io/ride
MIT License
204 stars 31 forks source link

Avoid CSS specificity race condition in language bar #1217

Closed TomaSajt closed 6 months ago

TomaSajt commented 7 months ago

I am in the process of packaging this app for Nixpkgs, and I found a "race condition" in the generated css.

I discovered this issue when trying to make the build process deterministic. The main reason for non-determisim was that some .less files had patterns like **/* in them, which is not something that is guaranteed to include all files in the same order every time. I came up with a hacky solution, which replaces **/* with code generation:

sed -i "/\*\*/d" layout.less light-theme.less dark-theme.less
find less/layout -name "*.less" -exec echo "@import '{}';" \; >> layout.less
find less/colour -name "*.less" -exec echo "@import '{}';" \; >> light-theme.less
find less/colour -name "*.less" -exec echo "@import '{}';" \; >> dark-theme.less

Having done this I noticed that the lower tooltips of the languagebar were now being shown in the Roboto font instead of the APL font.

It turns out #lb_tip_text (which is a pre tag) had two CSS rules with the same specificity acting on it. Both are trying to set the font.

https://github.com/Dyalog/ride/blob/c549c33119498fd6fdc34af2b5054f1eece4ad4f/style/less/layout/inputs.less#L38-L41

https://github.com/Dyalog/ride/blob/c549c33119498fd6fdc34af2b5054f1eece4ad4f/style/less/layout/setup.less#L2-L12

I could fix this issue by setting a higher specificity rule for the the tooltip. A possible solution:

#lb_tip_text {
  font: inherit;
}

However after inspecting the code, I could only find 1 other instance of pre being used: https://github.com/Dyalog/ride/blob/c549c33119498fd6fdc34af2b5054f1eece4ad4f/index.html#L255

It turns this also has a second rule acting on it, which overrides the font to monospace https://github.com/Dyalog/ride/blob/c549c33119498fd6fdc34af2b5054f1eece4ad4f/style/less/colour/connect_page.less#L68-L78

So I came to the conclusion that pre was never supposed to have the Roboto font.

(Also, why should a pre tag ever need to have Roboto as its font? "preformatted text" should have a monospace font by definition)

TomaSajt commented 6 months ago

find is using a not-alphabetic but deterministic file order. However using bash for loops with **/*.less is deterministic AND alphabetic. Doing this works, by chance. I'll close this PR as this is nothing important, but I still think the stylesheets should get some changes.