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

Invert header if primary is bright and background disabled #35666

Merged
merged 1 commit into from
Dec 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions apps/theming/lib/Controller/UserThemeController.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,14 @@

class UserThemeController extends OCSController {

protected string $userId;
protected ?string $userId = null;

private IConfig $config;
private IUserSession $userSession;
private ThemesService $themesService;
private ThemingDefaults $themingDefaults;
private BackgroundService $backgroundService;

/**
* Config constructor.
*/
public function __construct(string $appName,
IRequest $request,
IConfig $config,
Expand All @@ -73,7 +71,11 @@ public function __construct(string $appName,
$this->themesService = $themesService;
$this->themingDefaults = $themingDefaults;
$this->backgroundService = $backgroundService;
$this->userId = $userSession->getUser()->getUID();

$user = $userSession->getUser();
if ($user !== null) {
$this->userId = $user->getUID();
}
}

/**
Expand Down
15 changes: 10 additions & 5 deletions apps/theming/lib/Themes/CommonThemeTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
*/
namespace OCA\Theming\Themes;

use OCA\Theming\AppInfo\Application;
use OCA\Theming\Util;
use OCA\Theming\ImageManager;
use OCA\Theming\AppInfo\Application;
use OCA\Theming\Service\BackgroundService;
use OCA\Theming\Util;

trait CommonThemeTrait {
public Util $util;
Expand Down Expand Up @@ -82,9 +82,9 @@ protected function generatePrimaryVariables(string $colorMainBackground, string
* Generate admin theming background-related variables
*/
protected function generateGlobalBackgroundVariables(): array {
$user = $this->userSession->getUser();
$backgroundDeleted = $this->config->getAppValue(Application::APP_ID, 'backgroundMime', '') === 'backgroundColor';
$hasCustomLogoHeader = $this->util->isLogoThemed();
$isDefaultPrimaryBright = $this->util->invertTextColor($this->defaultPrimaryColor);

$variables = [];

Expand All @@ -95,8 +95,10 @@ protected function generateGlobalBackgroundVariables(): array {
// If primary as background has been request or if we have a custom primary colour
// let's not define the background image
if ($backgroundDeleted) {
$variables['--color-background-plain'] = $this->themingDefaults->getColorPrimary();
$variables['--color-background-plain'] = $this->defaultPrimaryColor;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is intentional.
We're in the admin section, we should use defaultPrimaryColor instead of primaryColor
If logged in, this var will be overruled below in the user section

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain their differences ?

Copy link
Member Author

@skjnldsv skjnldsv Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultPrimaryColor = admin defined, instance-wide
primaryColor = user, active for current session

$variables['--image-background-plain'] = 'yes';
// If no background image is set, we need to check against the shown primary colour
$variables['--background-image-invert-if-bright'] = $isDefaultPrimaryBright ? 'invert(100%)' : 'no';
}

// Register image variables only if custom-defined
Expand Down Expand Up @@ -125,12 +127,15 @@ protected function generateUserBackgroundVariables(): array {
&& $this->appManager->isEnabledForUser(Application::APP_ID)) {
$backgroundImage = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'background_image', BackgroundService::BACKGROUND_DEFAULT);
$currentVersion = (int)$this->config->getUserValue($user->getUID(), Application::APP_ID, 'userCacheBuster', '0');
$isPrimaryBright = $this->util->invertTextColor($this->primaryColor);

// The user removed the background
if ($backgroundImage === BackgroundService::BACKGROUND_DISABLED) {
return [
'--image-background' => 'no',
'--color-background-plain' => $this->themingDefaults->getColorPrimary(),
'--color-background-plain' => $this->primaryColor,
// If no background image is set, we need to check against the shown primary colour
'--background-image-invert-if-bright' => $isPrimaryBright ? 'invert(100%)' : 'no',
];
}

Expand Down
63 changes: 53 additions & 10 deletions cypress/e2e/theming/admin-settings.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const defaultBackground = 'kamil-porembinski-clouds.jpg'
describe('Admin theming settings', function() {
before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

Expand All @@ -53,7 +53,7 @@ describe('Change the primary colour and reset it', function() {
let selectedColor = ''
before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

Expand All @@ -79,7 +79,7 @@ describe('Change the primary colour and reset it', function() {
})

it('Undo theming settings', function() {
cy.resetTheming()
cy.resetAdminTheming()
})

it('Screenshot the login page', function() {
Expand All @@ -92,7 +92,7 @@ describe('Change the primary colour and reset it', function() {
describe('Remove the default background and restore it', function() {
before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

Expand Down Expand Up @@ -122,7 +122,7 @@ describe('Remove the default background and restore it', function() {
})

it('Undo theming settings', function() {
cy.resetTheming()
cy.resetAdminTheming()
})

it('Screenshot the login page', function() {
Expand All @@ -132,14 +132,57 @@ describe('Remove the default background and restore it', function() {
})
})

describe.only('Remove the default background with a bright color', function() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe.only('Remove the default background with a bright color', function() {
describe('Remove the default background with a bright color', function() {

🙄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now, everybody knows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take the walk of shame 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before(function() {
// Just in case previous test failed
cy.resetAdminTheming()
cy.resetUserTheming(admin)
cy.login(admin)
})

it('See the admin theming section', function() {
cy.visit('/settings/admin/theming')
cy.get('[data-admin-theming-settings]').scrollIntoView().should('be.visible')
})

it('Remove the default background', function() {
cy.intercept('*/apps/theming/ajax/updateStylesheet').as('removeBackground')

cy.get('[data-admin-theming-setting-file-remove]').click()

cy.wait('@removeBackground')
})

it('Change the primary colour', function() {
cy.intercept('*/apps/theming/ajax/updateStylesheet').as('setColor')

// Pick one of the bright color preset
cy.get('[data-admin-theming-setting-primary-color-picker]').click()
cy.get('.color-picker__simple-color-circle:eq(4)').click()

cy.wait('@setColor')
cy.waitUntil(() => validateBodyThemingCss('#ddcb55', ''))
})

it('See the header being inverted', function() {
cy.waitUntil(() => cy.window().then((win) => {
const firstEntry = win.document.querySelector('.app-menu-main li')
if (!firstEntry) {
return false
}
return getComputedStyle(firstEntry).filter === 'invert(1)'
}))
})
})

describe('Change the login fields then reset them', function() {
const name = 'ABCdef123'
const url = 'https://example.com'
const slogan = 'Testing is fun'

before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

Expand Down Expand Up @@ -196,7 +239,7 @@ describe('Change the login fields then reset them', function() {
})

it('Undo theming settings', function() {
cy.resetTheming()
cy.resetAdminTheming()
})

it('Check login screen changes', function() {
Expand All @@ -212,7 +255,7 @@ describe('Change the login fields then reset them', function() {
describe('Disable user theming and enable it back', function() {
before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

Expand Down Expand Up @@ -250,12 +293,12 @@ describe('User default option matches admin theming', function() {

before(function() {
// Just in case previous test failed
cy.resetTheming()
cy.resetAdminTheming()
cy.login(admin)
})

after(function() {
cy.resetTheming()
cy.resetAdminTheming()
})

it('See the admin theming section', function() {
Expand Down
100 changes: 77 additions & 23 deletions cypress/e2e/theming/user-background.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,8 @@ import type { User } from '@nextcloud/cypress'

const defaultPrimary = '#006aa3'
const defaultBackground = 'kamil-porembinski-clouds.jpg'
import { colord } from 'colord'

const validateThemingCss = function(expectedPrimary = '#0082c9', expectedBackground = 'kamil-porembinski-clouds.jpg', bright = false) {
return cy.window().then((win) => {
const backgroundColor = getComputedStyle(win.document.body).backgroundColor
const backgroundImage = getComputedStyle(win.document.body).backgroundImage
const invertIfBright = getComputedStyle(win.document.body).getPropertyValue('--background-image-invert-if-bright')

// Returning boolean for cy.waitUntil usage
return colord(backgroundColor).isEqual(expectedPrimary)
&& backgroundImage.includes(expectedBackground)
&& invertIfBright === (bright ? 'invert(100%)' : 'no')
})
}
import { pickRandomColor, validateBodyThemingCss } from './themingUtils'

describe('User default background settings', function() {
before(function() {
Expand All @@ -61,7 +49,7 @@ describe('User default background settings', function() {
})
})

describe('User select shipped backgrounds', function() {
describe('User select shipped backgrounds and remove background', function() {
before(function() {
cy.createRandomUser().then((user: User) => {
cy.login(user)
Expand All @@ -82,7 +70,7 @@ describe('User select shipped backgrounds', function() {

// Validate changed background and primary
cy.wait('@setBackground')
cy.waitUntil(() => validateThemingCss('#a53c17', background))
cy.waitUntil(() => validateBodyThemingCss('#a53c17', background))
})

it('Select a bright shipped background', function() {
Expand All @@ -94,7 +82,7 @@ describe('User select shipped backgrounds', function() {

// Validate changed background and primary
cy.wait('@setBackground')
cy.waitUntil(() => validateThemingCss('#56633d', background, true))
cy.waitUntil(() => validateBodyThemingCss('#56633d', background, true))
})

it('Remove background', function() {
Expand All @@ -105,7 +93,7 @@ describe('User select shipped backgrounds', function() {

// Validate clear background
cy.wait('@clearBackground')
cy.waitUntil(() => validateThemingCss('#56633d', ''))
cy.waitUntil(() => validateBodyThemingCss('#56633d', ''))
})
})

Expand All @@ -124,10 +112,9 @@ describe('User select a custom color', function() {
it('Select a custom color', function() {
cy.intercept('*/apps/theming/background/color').as('setColor')

cy.get('[data-user-theming-background-color]').click()
cy.get('.color-picker__simple-color-circle:eq(3)').click()
pickRandomColor('[data-user-theming-background-color]')

// Validate clear background
// Validate custom colour change
cy.wait('@setColor')
cy.waitUntil(() => cy.window().then((win) => {
const primary = getComputedStyle(win.document.body).getPropertyValue('--color-primary')
Expand All @@ -136,6 +123,73 @@ describe('User select a custom color', function() {
})
})

describe('User select a bright custom color and remove background', function() {
before(function() {
cy.createRandomUser().then((user: User) => {
cy.login(user)
})
})

it('See the user background settings', function() {
cy.visit('/settings/user/theming')
cy.get('[data-user-theming-background-settings]').scrollIntoView().should('be.visible')
})

it('Remove background', function() {
cy.intercept('*/apps/theming/background/custom').as('clearBackground')

// Clear background
cy.get('[data-user-theming-background-clear]').click()

// Validate clear background
cy.wait('@clearBackground')
cy.waitUntil(() => validateBodyThemingCss(undefined, ''))
})

it('Select a custom color', function() {
cy.intercept('*/apps/theming/background/color').as('setColor')

// Pick one of the bright color preset
cy.get('[data-user-theming-background-color]').click()
cy.get('.color-picker__simple-color-circle:eq(4)').click()

// Validate custom colour change
cy.wait('@setColor')
})

it('See the header being inverted', function() {
cy.waitUntil(() => cy.window().then((win) => {
const firstEntry = win.document.querySelector('.app-menu-main li')
if (!firstEntry) {
return false
}
return getComputedStyle(firstEntry).filter === 'invert(1)'
}))
})

it('Select a shipped background', function() {
const background = 'anatoly-mikhaltsov-butterfly-wing-scale.jpg'
cy.intercept('*/apps/theming/background/shipped').as('setBackground')

// Select background
cy.get(`[data-user-theming-background-shipped="${background}"]`).click()

// Validate changed background and primary
cy.wait('@setBackground')
cy.waitUntil(() => validateBodyThemingCss('#a53c17', background))
})

it('See the header NOT being inverted', function() {
cy.waitUntil(() => cy.window().then((win) => {
const firstEntry = win.document.querySelector('.app-menu-main li')
if (!firstEntry) {
return false
}
return getComputedStyle(firstEntry).filter === 'none'
}))
})
})

describe('User select a custom background', function() {
const image = 'image.jpg'
before(function() {
Expand All @@ -160,7 +214,7 @@ describe('User select a custom background', function() {

// Wait for background to be set
cy.wait('@setBackground')
cy.waitUntil(() => validateThemingCss('#4c0c04', 'apps/theming/background?v='))
cy.waitUntil(() => validateBodyThemingCss('#4c0c04', 'apps/theming/background?v='))
})
})

Expand Down Expand Up @@ -192,7 +246,7 @@ describe('User changes settings and reload the page', function() {

// Wait for background to be set
cy.wait('@setBackground')
cy.waitUntil(() => validateThemingCss(primaryFromImage, 'apps/theming/background?v='))
cy.waitUntil(() => validateBodyThemingCss(primaryFromImage, 'apps/theming/background?v='))
})

it('Select a custom color', function() {
Expand All @@ -211,6 +265,6 @@ describe('User changes settings and reload the page', function() {

it('Reload the page and validate persistent changes', function() {
cy.reload()
cy.waitUntil(() => validateThemingCss(selectedColor, 'apps/theming/background?v='))
cy.waitUntil(() => validateBodyThemingCss(selectedColor, 'apps/theming/background?v='))
})
})
Loading