XWIKI-21492: Underline inline links #2694
Conversation
* Added various UI to change values for misc accessibility prefs. TODO: add the backend part in XWikiUsersDocumentInitializer.java L104 and the actual useful part in stylesheets.vm L54
* Removed unused accessibility options from the prototype
* Added the parameter logic
* Tried out updating the less skin variables, still unfinished
* Added various UI to change values for misc accessibility prefs. TODO: add the backend part in XWikiUsersDocumentInitializer.java L104 and the actual useful part in stylesheets.vm L54
* Removed unused accessibility options from the prototype
* Added the parameter logic
* Tried out updating the less skin variables, still unfinished
* Removed the tentative for personnalized LESS parameters - font size here
* Added back the preference for text size (flawed implementation like what it used to be) * Fixed the place where we loaded the CSS
* Cleaning up PR
There was a problem hiding this comment.
Thank you very much for working on this. Unfortunately, I think this needs significantly more work before it can be merged. Apart from the comments I already added, there are the following issues:
- We need a migration to enable both options by default for users who had the old option enabled. I see two options how this could be done:
- By reading the value of the old option (the value won't be removed when it has been modified) and enabling the CSS also in this case and also setting the options by default to a value that corresponds to the old behavior when editing the profile.
- By using a proper migration that migrates the configuration on the upgrade. I don't know if we have any examples/best practices for this.
- Inline links are still ill-defined and won't lead to a consistent experience. For example, consider this example content:
here, the first link would be underlined, but the second won't, unless I'm mistaken. I'm really not happy with this inconsistency, in particular if this is our default behavior.
xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/ApplicationResources.properties
Outdated
Show resolved
Hide resolved
...-profile/xwiki-platform-user-profile-ui/src/main/resources/XWiki/UserProfileTranslations.xml
Outdated
Show resolved
Hide resolved
| @@ -37,7 +37,8 @@ | |||
| <syntaxId>plain/1.0</syntaxId> | |||
| <hidden>true</hidden> | |||
| <content>XWiki.XWikiUsers_displayHiddenDocuments.hint=The wiki contains documents that are not displayed by default. These hidden documents represent technical content, like application classes, configuration pages, macros, styles, scripts, etc. | |||
| XWiki.XWikiUsers_accessibility.hint=Extra accessibility will enable various visual enhancements like: bigger fonts, underlined links, etc. | |||
| XWiki.XWikiUsers_font_size.hint=Change the font size for the main text (in px). | |||
There was a problem hiding this comment.
From my understanding, there are only a few possible values (Regular, Large, Larger, Largest) and no pixel values, so I don't understand the "in px" comment. Also, I think this is missing translations for the values.
| & > .wikiinternallink, | ||
| & > .wikicreatelink { | ||
| & > a { | ||
| text-decoration: underline; |
There was a problem hiding this comment.
This seems strange, why are links in list items, definition lists etc. not underlined?
There was a problem hiding this comment.
I reused the selector we converged towards at the end of this previous PR. It can definitely still be improved, I'll try to make it a bit better, but from what I can remember there's no perfect solution because we can't 'see' text nodes in CSS.
There was a problem hiding this comment.
I haven't checked what kind of elements we have normally as parents of links, but in general this approach seems very fragile and error-prone to me. I would have preferred an approach where we underline links with a broad, generic selector (to, e.g., generically match content and panels) and then use specific selectors to remove the underline where we don't want it. Just to avoid that we have cases where users don't understand why one link in the content is underlined and another isn't.
This is my opinion, I don't know how others see this and if they would prefer either that potentially some links in the content aren't underlined or that potentially too many links are underlined, but that underlining is mostly consistent.
There was a problem hiding this comment.
I updated the selector in d2941bf .
I've split it in different sections with easy objectives:
- Ruleset for all links in content.
- Exceptions to add underlining outside of content
- Exceptions to remove underlining inside content
- Helper classes to "force" the decoration on an anchor. Note that this class only makes a difference with the
underline inline linkpreference chosen. Withall links underlinedorno link underlined, the user preference is predominant.
The advantage of this is that it's more consistent and easier to understand than the previous one. The disadvantage is that it's not really consistent with the inline link definition. We might need to update this selector soon to address leftover accessibility violations. There's some of those violations everywhere in the build right now so I can't test them all out...
| </use> | ||
| </class> | ||
| <property> | ||
| <cache>default</cache> |
There was a problem hiding this comment.
Are you sure this is working and also, e.g., updated when user preferences are saved? I think a more robust solution could be one of the following:
- Put the user preference into a URL parameter that is passed when the SSX is loaded.
- Transform the user preference into a data attribute or class on a central element (like body or even HTML) and use CSS selectors based on that. This has the advantage that other CSS can easily adapt its styles when it makes sense.
There was a problem hiding this comment.
The line at #2694 (comment) was in charge of reloading the SSX no matter the cache. I updated the cache policy to account for removing this line, disabling cache for this file.
Ideally we should only invalidate cache for this skin extension when the user preference change, but technically it's not that easy.
Addressed in 6b9d08c 👍
See https://up1.xwikisas.com/#i_R7F8zyuo9LADkeBg-vgw for a demo. In this demo, I just changed the font size since it's the easiest to spot out quickly, but it works the same for the link underlining.
...rofile/xwiki-platform-user-profile-ui/src/main/resources/XWiki/XWikiUserPreferencesSheet.xml
Outdated
Show resolved
Hide resolved
| <link href="$escapetool.xml($xwiki.getSkinFile('css/accessibility.css', true))" rel="stylesheet" type="text/css" media="all" /> | ||
| #end | ||
| ## User preferences stylesheet | ||
| #set($discard = $xwiki.ssx.use('XWiki.XWikiUserPreferencesSheet')) |
There was a problem hiding this comment.
This is not necessary when the SSX has "always" as "use".
There was a problem hiding this comment.
Addressed in 344d906 👍
| </dl> | ||
| <h1>$escapetool.xml($services.localization.render('platform.core.profile.section.accessibilityPreferences'))</h1> | ||
| <dl> | ||
| #set ($defaultValue = $xwiki.getXWikiPreference('font_size')) |
There was a problem hiding this comment.
Where should that value come from?
There was a problem hiding this comment.
I don't understand this comment.
Here, I followed the same pattern as other properties.
From what I understand, the value comes from the edited user preferences.
There was a problem hiding this comment.
From what I understand, the value comes from the edited user preferences.
No, this loads a value from the wiki configuration, i.e., it reads values from the XWiki.XWikiPreferences document - which has an accessibility field, but no font_size or underline. Actually, I think $!{xwiki.getUserPreference('accessibility')} reads this accessibility field from the wiki (or main wiki) configuration, too, when it is not set in the user profile/subwiki.
There was a problem hiding this comment.
In regards to that, I added the values in the Xwiki and space preferences in 32a7cca .
👍
...rofile/xwiki-platform-user-profile-ui/src/main/resources/XWiki/XWikiUserPreferencesSheet.xml
Outdated
Show resolved
Hide resolved
...rm-oldcore/src/main/java/com/xpn/xwiki/internal/mandatory/XWikiUsersDocumentInitializer.java
Outdated
Show resolved
Hide resolved
* Updated translations * Removed a useless improt from stylesheets.vm * Changed option order
* Fixed codestyle Co-authored-by: Michael Hamann <[email protected]>
* Fixed codestyle Co-authored-by: Michael Hamann <[email protected]>
* Added translations for the field values
* Updated the title level
* Removed the reference to the flamingo skin
* Changed the cache policy on the user preference file
* Updated the selector
We can't see the text nodes with CSS, so we should have the same behaviour for all 4 examples: From what I remember, we wanted to avoid the 4th one being underlined because it'd be really heavy as a default for users: However, if we don't underline the first one, pretty much nothing will be underlined and we still have the issue of underlined inline links... So for now I'll take the option that might be too much for some users: |
* Added backwards compatibility: if the old parameter is set to 'accessible' on the profile, it will set the old preferences
I'm not sure this is so important, from my understanding, barely anyone used it, and those who did can easily change it back to what they used to have (and that's an incentive to find out about the new granularity in the preferences). Nevertheless, I added logic to take into account the old parameter if it's set in 7e5d8f0. I'm wondering what would happen for users who had it enabled though: |
|
All reviews have been answered to. |
|
There was an interesting talk about accessibility at FOSDEM that argued that websites should respect the user's settings in the browser instead of introducing new controls. There was an explicit question at the end if it would be bad for a web application to introduce a font size control and the presenter kind of agreed that it would be better to just respect the font size that the user has configured in the web browser. Also, all web browsers allow changing the font size/zooming the website. Thinking about it again, I wonder if it wouldn't really be better to just respect what the user set in the browser's settings and make sure that the browser's zoom feature works properly in XWiki. |
I checked out our implementation: In my opinion, we can update/override this couple rules to allow an use of the user browser level font-size.
The result with a 16px browser default is the same, but this new config can adapt to browser level preferences. If the user decided to have a 32px default font-size, Regular option will be 28px instead of staying at 14px. In my opinion this is still good to give an extra layer of font size tweaking to the user. But yeah, not taking in account browser defined font-size was a lost opportunity. The issue with this is that we completely deprecate the use of |
* Removed the reliance on wiki defined font-size to prefer user defined font-size
|
I updated the base font-size strategy in 7bf473e Here is what it looks like in a few situations (Sandbox page with a bit of custom content, 2560*1440 display, 100% zoom level, Firefox): |
|
Discussion started on https://forum.xwiki.org/t/deprecating-the-font-size-base-colortheme-variable/14005 , opening back this PR for reviews when the discussion ends. |
|
After taking a step back, I think this addition is a bit far from the initial topic of the PR. |
This reverts commit 7bf473e.
* Removed the SSX * Added style directly in general.less * Added logic related to preferences in the template htmlheader.vm
|
@michitux Here is a demo of the updated code. As expected, this works as well as before, with the added advantage that we don't have to mess around with the cache of a SSX, so presentation reliably updated. |
...amingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/htmlheader.vm
Show resolved
Hide resolved
...rofile/xwiki-platform-user-profile-ui/src/main/resources/XWiki/XWikiUserPreferencesSheet.xml
Show resolved
Hide resolved
| <validationRegExp/> | ||
| <values>OnlyInlineLinks|Yes|No</values> | ||
| <classType>com.xpn.xwiki.objects.classes.StaticListClass</classType> | ||
| </underline> |
There was a problem hiding this comment.
I wonder if we shouldn't also remove the legacy accessibility property here.
There was a problem hiding this comment.
To be honest I'm not sure what's the exact purpose of this file (seems like a repeat with the content of XWikiPreferences.xml ), so I'd rather not break unknown things by only adding what seems to make sense adding and leaving the legacy content unchanged.
| We use classes that are added to the body itself in htmlheader.vm */ | ||
| // XWiki font size preference | ||
| &.preference-font-size-regular { | ||
| font-size: 140%; |
There was a problem hiding this comment.
I would have left the font size unchanged in this case, also to avoid introducing any regressions. For example, I see that when setting the font size in the theme customization, this value is also applied to the body element. So when you set the font size, e.g., to 20px in the theme customization, you'll have a font-size: 20px on body. This CSS would completely break this. Further, also all other font sizes are not relative to the font size set in the theme customization. When the font size in the theme is set to 20px, all these settings apart from the largest setting would actually decrease the font size.
There was a problem hiding this comment.
TLDR: Font size is a mess and I'll propose a best practice to try and make things better slowly and be able to someday transition to a system that is up to what would be expected from a modern webapp.
I would have left the font size unchanged in this case, also to avoid introducing any regressions. For example, I see that when setting the font size in the theme customization, this value is also applied to the body element.
Okay, so I just checked, and bootstrap uses two default hard coded values at different levels :/
There's a 10px on the html node, and font-size: @font-size-base; on the body node(with its value expected to be in px).
This makes little sense to me and IMO that's an issue with bootstrap 3.
The way things are now, the 10px on the html node is completely useless...
So when you set the font size, e.g., to 20px in the theme customization, you'll have a font-size: 20px on body.
With the solution above, the theme customization value would be overriden by the percentage which has more specificity.
Our font-sizes are defined in multiple ways:
- percentage, rems and ems: depend on the relative font size (native CSS)
0.9*@font-size-base: depend on the bootstrap value, preprocessed into absolute values in px.
The only point of setting @font-size-base on the body is to allow using relative units without breaking presentation.
So I guess the way I should implement this is with a percent on the bootstrap value... However I feel like even this solution would break some UIs somehow... And we can't use default browser defined preferences as a basis.
This is an overly complex solution and I dislike it a lot. I'll make a proposal to start uniformizing our code base font-sizes so that we don't get stuck in an interwoven web of dependencies going through ten style files and two fundamentally incompatible models next time we try to make a system change relative to font sizes.
I think the best solution for long term would be to override bootstrap code with something that makes sense with the modern CSS capabilities, this would provide code that's easier to understand and maintain on the long term.
In my opinion we should aim at:
- The only absolute units for font sizes are used on
:root(orhtml). - Every other font size should be defined using percentages , ems or rems
There was a problem hiding this comment.
The changes described here have been pushed on this PR in 09af3b4 👍
There was a problem hiding this comment.
I absolutely agree with you that we need to get rid of the current font size system and use one that is based on either a single font size or even the browser's font size. However, the problem is that this font size that is set by bootstrap is not only used for computing other font sizes but also for paddings and margins (search for @line-height-computed in the source code).
However, this will be a lot of work and I think we should first decide if we want to stay with Bootstrap 3 or not before we put more work into modernizing Bootstrap 3.
Ideally, the font size settings shouldn't be part of this PR. However, I see the problem that you need to split the accessibility settings in order to offer several options for underlining links.
There was a problem hiding this comment.
Ideally, the font size settings shouldn't be part of this PR. However, I see the problem that you need to split the accessibility settings in order to offer several options for underlining links.
As I wrote in my previous comment, maybe it would make sense to just drop the font size feature for now (requires a vote) and to simply replace the accessibility setting by the underline links setting.
There was a problem hiding this comment.
However, the problem is that this font size that is set by bootstrap is not only used for computing other font sizes but also for paddings and margins (search for @line-height-computed in the source code).
Thank you for highlighting this, I did not connect the dots here. It will be a pain moving, since the basic semantics of this system look skewed. This is kind of similar to a regression we had earlier this month because we used a foreground color as a background :) .
However, this will be a lot of work and I think we should first decide if we want to stay with Bootstrap 3 or not before we put more work into modernizing Bootstrap 3.
From what I understood, the only reason why we stay on bootstrap 3 is the cost of migration (without it I'm pretty sure we'd have moved to the latest bootstrap in an heartbeat :) ). My idea would be to be less reliant on bootstrap 3 for font-size, which would make it easier to move to another system later.
Ideally, the font size settings shouldn't be part of this PR. However, I see the problem that you need to split the accessibility settings in order to offer several options for underlining links.
👍 It's a bit of a pain but it's something I would have added not long after anyways if it wasn't necessary to avoid a regression on features.
There was a problem hiding this comment.
I would have left the font size unchanged in this case, also to avoid introducing any regressions. For example, I see that when setting the font size in the theme customization, this value is also applied to the body element. So when you set the font size, e.g., to
20pxin the theme customization, you'll have afont-size: 20pxonbody. This CSS would completely break this. Further, also all other font sizes are not relative to the font size set in the theme customization. When the font size in the theme is set to20px, all these settings apart from thelargestsetting would actually decrease the font size.
By the way I just realized that the legacy way to implement the accessible mode also overrode the @font-size-base defined in the colortheme and bootstrap :)
It set body{ font-size: 100% !important}.
There was a problem hiding this comment.
My comment here was only regarding the "regular" size, "this case" meant the default "regular" font size. My worry here was primarily that the configured font size should be respected and left unchanged as much as possible when the font size setting is on the default value.
There was a problem hiding this comment.
The point that the accessible font size could end up smaller than the regular one was already a thing though
There was a problem hiding this comment.
To make things even easier to understand, CKEditor uses @base-font-size, yet another value with a similar name and probably a different system behind it.
| } | ||
|
|
||
| &.preference-font-size-largest { | ||
| font-size: 200%; |
There was a problem hiding this comment.
I'm wondering if 200% isn't breaking the layout too much currently.
There was a problem hiding this comment.
I updated it to 1.4*@font-size-base.
Note that this 200% wasn't actually 200% of the default size but 200/140, which was only a 42% increase.
With a @font-size-base of 20px (above default), zoom level 100%, on a 2k display I get:
I used the Firefox device size emulation dev tool to get this display in the same conditions for a 1920*1080 screen:
IMO this looks okay, a bit off in some places but not breaking the layout.
Note that for most users, the text will be smaller than that, this is a worst case scenario.
There was a problem hiding this comment.
This was done with the changes pushed in 09af3b4
There was a problem hiding this comment.
To me the whole spacing just looks completely wrong. Also, the fact that not all text is scaled and in particularly the smallest text is not scaled at all makes this feature kind of useless to me.
I'm wondering if we shouldn't just have a vote to drop this font size increase feature in favor of pointing users to the browser's zoom functionality until we have proper font scaling support.
There was a problem hiding this comment.
I figured out why... The values with 0.9*@font-size-base would not be modified by this...
E.g. the last modified is the same size whatever the preference.
We'd need to have the value of font-size-base change based on the preferences, but AFAIK this is not something we want to do because we would need to recompile all the LESS of the codebase for each user... which was the exact thing we tried to avoid by using a class on the body node...
So 👍 to start a vote, and implement this font size feature back when we have a codebase consistent enough to make it possible. I'll start it tomorrow morning :)
There was a problem hiding this comment.
I think what might be worth investigating is if we could replace the less variable @font-size-base by a CSS variable without changing the whole logic too much. This might allow easily changing the base font size with full effects everywhere without needing any less compilation.
There was a problem hiding this comment.
So, I just checked it out, and LESS is not really made with that in mind...
First, I came up with escaping issues, the -- used to define a CSS variable were interpreted as the start of --> and failed the LESS compilator, so I had to escape all of those characters:
#elseif($property == 'font-size-base')
## Special case, we rely on a CSS variable to control the value so that we can update it with more ease.
:root {\-\-$property: $value;}
@$property: var(\-\-$property);
in variablesInit.vm
and
&.preference-font-size-regular {
\-\-font-size-base: calc(1*var(\-\-font-size-base));
}
in general.less
However, this workaround was not enough, because after that compilation failed with the message:
Caused by: com.github.sommeri.less4j.Less4jException: Could not compile less. 270 error(s) occurred:
ERROR less/bootstrap/variables.less 67:34 Wrong argument type to function 'floor', expected number saw LIST_EXPRESSION.
66: //** Computed "line-height" (font-size*line-height) for use withmargin,padding, etc.
67: @line-height-computed: floor((@font-size-base * @line-height-base)); // ~20px
68:
Bootstrap uses the value of the @font-size-base to calculate the value of @line-height-computed, but uses LESS syntax to do so. This syntax is not compatible with CSS variables 😞
There was a problem hiding this comment.
Bootstrap uses the value of the @font-size-base to calculate the value of @line-height-computed, but uses LESS syntax to do so. This syntax is not compatible with CSS variables 😞
My idea would have been to also replace @line-height-computed by a variable that is initialized as CSS calc() expression.
* Added an unsetter for the accessibility parameter, and defaults for the new parameters that would get changed if the accessibility parameter is already set. * Used `@font-size-base` instead of the user preferences as the basis for body fonts.
|
Started a vote at https://forum.xwiki.org/t/drop-the-font-size-increase-preference/14241 |
* Removed changes related to font-size from the PR
|
Vote ended with two +0. |
There was a problem hiding this comment.
I have still some smaller comments and I wonder if we shouldn't really remove the accessibility preference from XWiki.XWikiPreferences. And then of course this probably needs more testing to find all the edge cases where links are underlined that shouldn't be underlined.
xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/ApplicationResources.properties
Outdated
Show resolved
Hide resolved
...amingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/htmlheader.vm
Outdated
Show resolved
Hide resolved
| #set ($preferenceUnderlining = $preferenceUnderlining + "yes") | ||
| #elseif($underlining == 'OnlyInlineLinks' && $a11y != '1') | ||
| #set ($preferenceUnderlining = $preferenceUnderlining + "only-inline-links") | ||
| #elseif($underlining == 'No' && $a11y != '1') |
There was a problem hiding this comment.
This should be a simple #else to avoid that if $preferenceUnderlining is empty, nothing is added to the string. And I think it would actually be best to make the "only inline links" option the last one to make sure that it is the default if for some reason $underlining doesn't have one of the expected values.
There was a problem hiding this comment.
Addressed in df185ba 👍
...-profile/xwiki-platform-user-profile-ui/src/main/resources/XWiki/UserProfileTranslations.xml
Show resolved
Hide resolved
* Removed leftover translations
* Deprecated a translation key instead of completely removing it.
Removing it could be considered a backward compatibility breakage. If a custom content relies on this preference to provide its own accessibility hacks, it would completely break it. IMO we should at least keep it in the backend for a while, even if we remove all the UI related to it and override its default behavior with new preferences. Eventually if we find out that this first step didn't stir any trouble, we could consider removing it completely.
Some additionnal manual testing is necessary 👍 I looked at a few pages but for obvious reasons I couldn't try all use cases. I'll keep an eye out for those once it's merged, and put details about this situation in a release note for devs. IMO what we have now is good enough to solve the ticket and anything we report afterward can have its own issue (hopefully nothing critical enough to be categorized as a bug :) ). |
You also removed the preference from the user profile. Removing the property doesn't remove any changed values, it just removes the value from the class so you won't be able to set it anymore. But to me that part isn't that critical, I have no idea if we have any official policy how to handle this. |
* Fixed the preference conditions for underlining preferences
Jira
https://jira.xwiki.org/browse/XWIKI-21492
PR Changes
Notes
!importantthat are now against our codestyle.View
Here is the new default: Only inline links underlined and regular font size
Hereafter are a few of the variations:
Only inline links underlined and large font size
Only inline links underlined and larger font size
Only inline links underlined and largest font size
No link underlined and regular font size (same as the default before this PR)
All links underlined and regular font size
Hereafter are the view and edit modes of the updated user preference section:
