Fixed Logo not showing with 'accessability' themes

[fuchsi3010]

hack-y (?) fix for #26578
fixed the relative path to the img directory in the accessability .scss files.
i'm not sure if this is the best way to fix this, please refer to the discussion at the issue.

I tested the fix with the dark theme on my nextcloud instance running Nextcloud 21.0.1 on docker.
I did not test with the other themes.

[fuchsi3010]

the DCO check didn't go through, i hope i fixed it now

[MichaIng]

Many thanks for opening the PR. I verified that the overrides need to be done in all four style sheets, as enabling any of the accessibility flags (dark mode, high contrast, dyslexia font) alone or in combination is changing the style variable to point to a logo.svg inside the accessibility app directory.

Did you or could you test whether using the theming app to change the logo, in combination with an accessibility flag, still works?

And yes, it is a bit hacky without knowing the underlying idea/concept behind these (logo) variables, why the style root directory is changed at all by the theming app and whether this can be done more selectively for the shipped style sheets only in the first place etc. But it works.

[fuchsi3010]

Did you or could you test whether using the theming app to change the logo, in combination with an accessibility flag, still works?

I tested on my Nextcloud 21.0.1 (docker), with a PNG and a SVG image for the logo in the theming app, and they both worked with my modified dark theme from the accessability app.

[MichaIng]

Okay, let's run the static tests. I can only confirm that it is working like that, but since it looks more like a symptom-side fix, John (the app maintainer) should have a look whether there is a fix possible that grabs the issue by its root 🙂.

[skjnldsv]

Tests are saying no

[MichaIng]

Psalm errors are unrelated to this PR. I triggered a rerun of the Lint checks, as they only show "GitHub Actions has encountered an internal error when running your job.". Not sure if probably the logs have been removed in the meantime, so we need to review faster?

[MichaIng]

@skjnldsv
Tests are now saying yes 🙂. Can you have a look?

It's actually quite similar to f1f113c, where as well an asset path is manually overwritten to use a core asset. Not sure whether !default makes sense.

[szaimen]

/rebase

[MichaIng]

/rebase

This is from a fork. @fuchsi3010 would you mind to rebase this commit onto the current master?

[fuchsi3010]

hi,
ad @MichaIng: i wanted to remove the '!default' for a while and took the opportunity to do it now.
the '!default' doesnt seem to have an effect, i think i copied it from the 'main' CSS file and worked from there.
sorry for the late change.

ad rebase: I'm a git noob, this is what i did:

mkdir src && cd src
git clone git@github.com:fuchsi3010/server.git
git checkout -b css-reroute

then i made the changes (first time around i edited straight to fuchsi3010/server master branch)
note: i did edit dark.scss, highcontrast.scss and fontdyslexic.scss, but highcontrastdark.scss can remain unchanged.
with this, all the different combinations of accessibility themes work in my Nextcloud 22.1.1 stable on docker.

git commit -a
git checkout master
git pull git@github.com:nextcloud/server.git
git checkout css-reroute
git rebase master
git checkout master
git push

i hope i did everything alright, thanks for your patience! :-)

[fuchsi3010]

ok that didn't work as intended :-( made the changes again and pushed, seems ok now.
also noticed i could have fetched changes from the github web-gui... oops.

edit: didn't sign the commit, jfc

[MichaIng]

I think the fix is missing now for the highcontrastdark.scss which is still the only single CSS used when selecting high contrast as well as dark mode in accessibility, isn't it?

I do rebase like that:

git remote add upstream 'https://github.com/nextcloud/server.git'
git fetch upstream
git rebase upstream/master
git commit --amend --no-edit # Currently not sure if this is even required
git push -f origin HEAD

But your way worked as well, while generally you should be able to skip merging thinks between css-reroute and master branch and use your local master branch only?

[fuchsi3010]

Any way i try it (resetting cache, private mode, different browser, server restart) i can't get the logo to disappear, with dark & highcontrast enabled in the Accessability App.
In a 'minimalist' approach to this hack-y fix, i would suggest the least amount of changes necessary to reach the result.
Can anybody reproduce the missing logo with the current changes?

@MichaIng Thanks the detailed reply! I thought, maybe it would be easier if all of 'my' changes were on a single branch... but i guess rebasing scraps that anyway? i'll do a bit more reading-up on git when i get the time :-)

[MichaIng]

This is strange, I wonder if this changed the behaviour that the combined high contrast and dark mode is not used anymore. On the other hand it should only have an effect on guests (no logged in user) and only sets OCA.Accessibility.theme while OCA.Accessibility.highcontrast remains the same. Here is the code responsible to selecting the highcontrastdark CSS: https://github.com/nextcloud/server/blob/215aef3/apps/accessibility/lib/Controller/AccessibilityController.php#L119-L126
I haven't tried to understand how those are connected 😅.

Can you verify that when you select high contrast and dark mode, that the --color-main-text variable is set to #fff, like here:

In my case it's #d8d8d8 from dark mode only, in case of high contrast only it's #000. So this is a reliable way to check which SCSS has been used 😄. If high contrast + dark mode indeed shows --color-main-text: #000, then the expected CSS was loaded, but then the icon should be currently broken without the fix being applied to highcontrastdark.scss.

[fuchsi3010]

i have both dark + highcontrast enabled and --color-main-text is #FFF

(this is firefox developer tools, not sure what you are using?)

how would one choose the theme without logging in? is it a browser setting?

[MichaIng]

Okay so the correct CSS is loaded, and the logo is still present? The variables should then actually still point to the invalid path with high contrast + dark theme 🤔.

The guest fix is based on a browser/media feature prefers-color-scheme: dark, I guess like when you select dark mode in newer Android versions, and also my browser has a flag which may trigger it: https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme
With this set to "dark" the accessibility app dark mode is not automatically applied for guests.

[fuchsi3010]

ok, i checked both dark and highcontrast checkboxes, reloaded the page.
filtering for logo.svg?v=1, network monitor in firefox devtools shows me 200 and GET, so i conclude it was not cached.
Request Headers:

GET /core/img/logo/logo.svg?v=1 HTTP/2
Host: [REDACTED]
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101 Firefox/92.0
Accept: image/webp,*/*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Connection: keep-alive
Cookie: __Host-nc_sameSiteCookielax=true; __Host-nc_sameSiteCookiestrict=true; nc_username=fuchsi3010; nc_token=[REDACTED]; oc_sessionPassphrase=[REDACTED]
Sec-Fetch-Dest: image
Sec-Fetch-Mode: no-cors
Sec-Fetch-Site: same-origin
Cache-Control: max-age=0
TE: trailers

Response Headers:

HTTP/2 200 OK
accept-ranges: bytes
cache-control: max-age=15778463
content-type: image/svg+xml
date: Mon, 20 Sep 2021 13:06:22 GMT
etag: "141-5cad15803bb26"
last-modified: Tue, 31 Aug 2021 02:02:37 GMT
referrer-policy: no-referrer
server: Apache/2.4.48 (Debian)
x-content-type-options: nosniff
x-download-options: noopen
x-frame-options: SAMEORIGIN
x-permitted-cross-domain-policies: none
x-robots-tag: none
x-xss-protection: 1; mode=block
content-length: 321
X-Firefox-Spdy: h2

the inspector shows:

:root {
    --color-main-text: #fff;

so i think it loads the correct one?

as i said, my approach would be: this is a hacky fix, so i want to make as little changes as necessary.
anything i tried with the current changes could not yield a missing logo on NC 22.1.1 .

but if you want me to add the lines to highcontrastdark.scss i'll do it right away.
just say your preference please, whatever is easiest to close this PR finally.

[MichaIng]

Strange, then probably all three (or two) of these CSS are loaded now or they are merged in the first place. If it works, it works 🙂.

Yes it is hacky somehow, but there are other such hacky things in these stylesheets. It is generally not clean that these accessibility themes are treated as themes but are in fact no complete themes. Internally the variables are set as if all required theme files were present, but they are not. Best would be a native way to have the variables changed only for shipped files and else fall back to core assets automatically.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22