learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
755 stars 637 forks source link

Fix pdf css import #12348

Closed AlexVelezLl closed 6 days ago

AlexVelezLl commented 6 days ago

Summary

In #9698 the way vendored pdfjs css was being imported changed from @import url('..'); to @import '...';, and for some reason, this new way of importing css wasnt taking into account the nested css selectors as .text-layer span when it was in a .scss file. Changing these vendored files to be a .css file made these nested selectors to work. I checked that it was safe to change the extension of these files since the syntax is compatible with CSS and no SCSS syntax was being used.

Reviewer guidance

Open any pdf file and confirm that the text is still selectable and no visual errors appear.

Before

image

After

image


Testing checklist

PR process

Reviewer checklist

github-actions[bot] commented 6 days ago

Build Artifacts

Asset type Download link
PEX file kolibri-.pex
Windows Installer (EXE) kolibri-0.17.0a0.dev0+git.117.g81fe8a4d-windows-setup-unsigned.exe
Debian Package kolibri_0.17.0a0.dev0+git.117.g81fe8a4d-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.17.0a0.dev0+git.117.g81fe8a4d-0.4.2.dmg
Android Package (APK) kolibri-0.17.0a0.dev0+git.117.g81fe8a4d-0.1.3-debug.apk
TAR file kolibri-0.17.0a0.dev0+git.117.g81fe8a4d.tar.gz
WHL file kolibri-0.17.0a0.dev0+git.117.g81fe8a4d-py2.py3-none-any.whl
rtibbles commented 6 days ago

From my note that introduced this change:

'import-notation': 'string', // Enforce string imports rather than 'url' as 'url' doesn't work with sass-loader

Interested to know why this is breaking sass imports, when the change was made specifically for sass imports (I am fine with the fix here, but feels like something that might gotcha people in the future too).

AlexVelezLl commented 6 days ago

@rtibbles I have been investigating deeper, and it seems that this bug is caused by scopes and how the CSS files are compiled. When we compile the selector .text-layer span as scss, the resulting selector is .text-layer span[data-v-5625f7f5], and it doesn't match the rendered node since the span tag is not rendered by vue but by PDF.js, and it doesn't have the data-v-5625f7f5 flag.

But if we import it as a css file, the selector doesnt change, and it remains as .text-layer span, so that is why it works. But it makes me think that we have to be careful with the selectors coming from css files because they have no scope.

A resulting selector with scope that should work would be .text-layer[data-v-5625f7f5] span since the .text-layer node is rendered by vue. And just got it working by adding the /deep/ keyword in the selector, so .text-layer /deep/ span => .text-layer[data-v-5625f7f5] span, and it seems to fix the issue too. It makes more sense to me to add these /deep/ keyword in the selectors, but it could potentially cause some regressions, since it seems that we have allways been using non-scoped selectors for pdf renderer css vendored files. what do you think?

rtibbles commented 6 days ago

I think rather than using /deep/ selectors, it might be better to import these files into an unscoped style block, interestingly, we don't have similar issues for our vendored perseus styles being in a scoped style block: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/perseus_viewer/assets/src/views/PerseusRendererIndex.vue#L696

Nor for our videojs vendoring: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/media_player/assets/src/views/MediaPlayerIndex.vue#L597

AlexVelezLl commented 6 days ago

So we can just left the changes that are currently in the PR right? @rtibbles. Since importing css files (instead of scss files), are already unscoped styles.

It seems that it doesnt matter if they are being imported in scoped style block, all css's imported files are unscoped. This particular issue was because we had these vendored files as scss

rtibbles commented 6 days ago

Yes - no problem with these changes - may be worth documenting this gotcha somewhere, although I'm not precisely sure where!