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

Makes the addon selector more "themeable" #566

Merged
merged 2 commits into from Feb 23, 2022

Conversation

dgdavid
Copy link
Member

@dgdavid dgdavid commented Feb 4, 2022

TL;DR

Read #566 (comment)

The Long Story

While working on a light theme, @jhmarina couldn't style the "Extension and Module Selection" dialog for a registered system, ending up with an almost empty (and not usable at all) screen.

Why?

Because the styles for the checkboxes are neither, in the .qss nor the richtext.css file. Instead, they are hard-coded in the HTML built for simulating them.

Wait, HTML?

Yes, HTML. Contrary to the dialog used for displaying the extensions/modules for a no registered system, which uses a MultiSelectionBox, when the system is registered the dialog uses a RichText to simulate the multi selection with more than two (on/off) statuses. Interesting and clever idea, isn't it? For reading more details about the reasoning behind it, read following links:

Solution

At first sight, the fix looks simple: just move those hard-coded styles to the corresponding (installation_)richtext.css file, which is read by the Y2Styler and set as a default CSS document for the RichText before adding its content.

Unfortunately, the Qt's rich text engine only supports a limited HTML and CSS subset, which basically means that this solution will work only partially.

Text color and decoration

Since color and text-decoration CSS properties are supported, the styles for the checkbox caption can be safely moved to the mentioned CSS file, no matter if it is surrounded by an a or span element. In fact, this is exactly the change that this PR proposes: replacing the style='text-decoration:none; color:#{color}' and style='color:grey' by class='enabled-item' and class='disabled-item' respectively. Better class names are welcome, of course.

It should work as long as https://github.com/yast/yast-theme/ and https://github.com/openSUSE got the needed CSS (PRs not created yet).

/* For the installation_richtext.css */
.enabled-item {
  color: white;
  text-decoration: none;
}

.disabled-item {
  color: gray;
}

/* For the richtext.css */
.enabled-item {
  color: black;
  text-decoration: none;
}

.disabled-item {
  color: gray;
}

Checkbox image

Due to mentioned limitations, the checkbox image cannot be moved to the CSS file (read, @dgdavid was not able to do it). Let's recap tests done:

  • Using a span or div element

    It will not work because display, width, and height CSS properties are not supported

  • Still using the img without the src attribute but with a class instead

    <img class="fake-checkbox">

    Does not work, because Qt renders a file icon somehow meaning "missing file" or so when the src is not present, wrong, or empty.

  • Ok, use a transparent pixel as src

    <img class="fake-checkbox" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII=">

    Good try, but remember: width and height CSS properties are not supported.

  • Then, use the supported width and height img attributes.

    <img class="fake-checkbox" width="18" height="18" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII=">`

    Great! It even opens the door to use them for setting a reasonable big value (18x18, maybe?) and leave the checkbox size up to the designers (14x14? 15x15? 12x12?). Unluckily, background-position and background-repeat CSS properties aren't supported either.

  • Never mind, tell them a fixed size

    <img class="fake-checkbox" width="14" height="14" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII=">`

    It does not work. Looks like Qt places the center of the image as the origin (??), repeating it until filling the placeholder reserved for it.

See the screenshot for a visual summary

Visual summary about why checkbox image cannot be moved to the richtext CSS file

Anyway, there are other extra disadvantages with the background-image approach

  • Designers must know the image location in the file system (not a big deal, though)
  • When the user selects the RichText content, checkboxes disappears (because of the transparent pixel)

Checkbox image, the workaround

Fortunately, there is a quick workaround for making those checkboxes visible in a light theme too: changing the border of inst_checkbox-off.svg from white to gray. It will work unless the theme uses a gray background, obviously.

See the screenshots below

Dark theme Light theme
Unselected checkboxes in dark theme Unselected checkboxes in light theme

However, still being not perfect (that's why is called workaround): designers cannot change/adapt the checkbox color to the theme. Imagine a dark theme with green checkboxes and a light theme using orange ones instead.

Dark theme Light theme
Dark theme using green checkbox Light theme using orange checkbox

Unless we can provide a solution for loading the icons per theme. E.g., /usr/share/YaST2/theme/sle/dark-installation/icons or /usr/share/YaST2/theme/sle/light-installation/icons or so. Even falling back to a default one in case of file missing (?) /usr/share/YaST2/theme/sle/installation/icons

For this, themes need a name or identifier, of course. Perhaps something to have in consideration in the context of https://trello.com/c/joXRejJi (internal link).

Additional information

This is not the first time we have issues with those dialogs/selectors. Read #483 (specially "The Long Story" chapter), #484, and #487 to know more.

More additional information (2022-02-23)


DISCLAIMER: I (@dgdavid) do not like the trick of using the transparent pixel. I was just exploring the ways for moving the hard-coded styles to a CSS file. Moreover, although I strongly admire the work and effort done back in the time for providing solutions with the tools at hand, I don't like the emulated multi selector either. IMHO, a native libyui solution is the right way to definitely improve these dialogs.

@coveralls
Copy link

coveralls commented Feb 4, 2022

Coverage Status

Coverage decreased (-0.003%) to 84.409% when pulling 241c3dd on make-addons-selector-themeable into 8a6d8b3 on master.

@dgdavid
Copy link
Member Author

dgdavid commented Feb 7, 2022

As asked by @kobliha in IRC, a grep of style= in YaST Ruby files returns following results

➜ ag " style\=" --ruby    

hana-update/data/tmpl_restore_cluster_con.erb
45:        [<a style="color:#00843E;font-weight:bold" href="revert_sync_direction">Reverse</a>]<br/>

hana-update/data/tmpl_restore_cluster.erb
28:        [<a style="color:#00843E;font-weight:bold" href="revert_sync_direction">Do not reverse</a>]<br/>
47:        [<a style="color:#00843E;font-weight:bold" href="revert_sync_direction">Reverse</a>]<br/>

helper_scripts/jekyll/lizards_html.rb
66:    "alt=\"&#x#{code};\" class=\"wp-smiley\" style=\"height: 1em; max-height: 1em;\" />"
71:    "alt=\"&#x#{code};\" class=\"wp-smiley\" style=\"height: 1em; max-height: 1em;\" />"

helper_scripts/jekyll/lizards_rss.rb
49:        "alt=\"&#x#{code};\" class=\"wp-smiley\" style=\"height: 1em; max-height: 1em;\" />"
54:        "alt=\"&#x#{code};\" class=\"wp-smiley\" style=\"height: 1em; max-height: 1em;\" />"

registration/src/lib/registration/ui/addon_selection_base_dialog.rb
185:            "<a href='#{id}' style='text-decoration:none; color:#{color}'>#{widget}</a>"
187:            "<span style='color:grey'>#{widget}</span>"

@lslezak
Copy link
Member

lslezak commented Feb 7, 2022

Ok, so only registration and hana-update are affected...

@joseivanlopez
Copy link
Contributor

joseivanlopez commented Feb 7, 2022

Thanks @dgdavid for this great summay! And thanks for clarfiying me why it is not possible to specify different checkbox images for each stylesheet. Let me write it here just in case anyone else has the same doubts:

  • Registration::UI::AddonSelectionBaseDialog emulates checkboxes inside a RichText.
  • It uses images to emulate a checkbox, and there are images for the different possible states of the checkbox: selected/deseleted or enabled/disabled.
  • The images to use are not especified in a qss or css stylesheet. The name of images are hard-coded in the Registration::UI::AddonSelectionBaseDialog class.
  • For each fake checkbox, Registration::UI::AddonSelectionBaseDialog decides the image to use. For that, it consideres the status of that fake checkbox (enabled, selected, etc) and the YaST mode (installation or normal).

So, as David already exposed before, the best solution could be to replace these fake checkboxes for a real widget provided by libyui. A less disruptive approach could be to extend the criteria of selecting the checkbox images and start considering the currently selected theme too.

dgdavid added a commit that referenced this pull request Feb 23, 2022
This requires changes in the corresponding theme richtext.css file, but
allows designers to choose the desired addon text color avoiding
usability/visibility problems in certain themes.

Read #566 to understand
why.
@dgdavid dgdavid force-pushed the make-addons-selector-themeable branch from 55262e2 to ce70264 Compare February 23, 2022 11:36
@dgdavid dgdavid marked this pull request as ready for review February 23, 2022 11:36
This requires changes in the corresponding theme richtext.css file, but
allows designers to choose the desired addon text color avoiding
usability/visibility problems in certain themes.

Read #566 to understand
why.
@dgdavid dgdavid force-pushed the make-addons-selector-themeable branch from ce70264 to 50076ec Compare February 23, 2022 11:43
package/yast2-registration.changes Outdated Show resolved Hide resolved
@shundhammer
Copy link
Contributor

Just for completeness: I asked in IRC if this was tested with NCurses, but that part of the code is only executed for the Qt UI.

Copy link
Contributor

@shundhammer shundhammer left a comment

Choose a reason for hiding this comment

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

LGTM

dgdavid added a commit to yast/yast-theme that referenced this pull request Feb 23, 2022
Which helps (slightly and) easily adapt the style of the addons
selector. See yast/yast-registration#566 for
more context and details.
dgdavid added a commit to yast/yast-theme that referenced this pull request Feb 23, 2022
To make the emulated not selected checkbox visible in both, a dark and a
light theme. Check yast/yast-registration#566
for more context and details.

The SVG has been optimized too, making use of the online
https://jakearchibald.github.io/svgomg/ tool.
@dgdavid dgdavid force-pushed the make-addons-selector-themeable branch from 50076ec to 241c3dd Compare February 23, 2022 15:24
@dgdavid dgdavid merged commit 65b3330 into master Feb 23, 2022
@dgdavid dgdavid deleted the make-addons-selector-themeable branch February 23, 2022 16:48
@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #105 successfully finished
✔️ Created IBS submit request #265881

@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #142 successfully finished
✔️ Created OBS submit request #957128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants