Page MenuHomePhabricator

2017 wikitext editor preview and visual diff has wrong line-height and font-size on Vector
Open, Needs TriagePublicBUG REPORT

Description

The font-size and line-height of the text in the visual editor and the 2017 wikitext editor has not the same as the parser output shown on the actual page when using the Vector skin.

The class the applies the font-size and line-height values used by the text shown on page is vector-body, which is applied to the #bodyContent element.

The issue is that the overlay that contains the preview when using the visual editor and/or the 2017 wikitext editor is located outside of this element, so the correct styles are not applied (although the rule for font-size is also added by the ve-ui-overlay-global class).

image.png (123×535 px, 15 KB)
image.png (148×579 px, 15 KB)
image.png (185×582 px, 26 KB)
The current previewThe preview with the style correctedThe actual page

Event Timeline

BrandonXLF renamed this task from 2017 wikitext editor has wrong line height and font size on Vector to 2017 wikitext editor has wrong line-height and font-size on Vector.Jul 30 2021, 7:56 AM
matmarex renamed this task from 2017 wikitext editor has wrong line-height and font-size on Vector to 2017 wikitext editor preview has wrong line-height and font-size on Vector.Aug 26 2021, 3:31 PM
matmarex renamed this task from 2017 wikitext editor preview has wrong line-height and font-size on Vector to 2017 wikitext editor preview and visual diff has wrong line-height and font-size on Vector.
matmarex added subscribers: Jdlrobson, matmarex.

Caused by rSVEC3306124038e5: Introduce the vector-body class, which changed various typography-related styles to only apply to .vector-body (a new selector specific to Vector), rather than .mw-body-content (a selector used by all skins, and VisualEditor). .mw-body would also work.

There was a similar issue with Vector affecting the main VisualEditor editing area only a few months ago in May: T283014: Font and font-sizes broken in VE.

vector-body is not the same as mw-body-content. The former applies to all elements in the content area, which includes categories, and subtitles and the latter is a standardized class that should mean the same across all skins. I wouldn't advise mw-body as this is not standard at current time and might change in future.

I can advise what's the most future-proof thing to do here, and what to do to fix it, but I'm not too familiar with the 2017 wikitext editor and what is doing (I'm only aware of 2011). How do I enable it? Can you please add some replication steps?

It's a beta feature, https://en.wikipedia.org/wiki/Special:Preferences#mw-prefsection-betafeatures → "New wikitext mode". You can also reproduce using visual diff in visual editor.

ppelberg added a subscriber: ppelberg.

META
@Jdlrobson: I'm boldly assigning this to you with the assumption that you're going to work on this fix.

If I've assumed incorrectly, please let me know.

@ppelberg I am probably not the best person to work on this fix. I have little experience with OOUI or this product I can certainly provide code review support and test this if necessary.

The fix should be in the wikitext editor code. The most future-proof approach here is to copy across classes from the parent item of the standard #mw-content-text to the new item. A better long-term solution would be to have some kind of core API for creating content area, but I don't have bandwidth to think about that too deeply right now.

$('.ve-ui-overlay-global .mw-body').addClass( document.getElementById('mw-content-text').parentNode.getAttribute('class') )

I don't know anything about this code so I'm not sure what the best way of doing this is.

@ppelberg I am probably not the best person to work on this fix. I have little experience with OOUI or this product I can certainly provide code review support and test this if necessary.

Understood, okay. This context is helpful to have...than you, @Jdlrobson.

The Editing Team will take responsibility for fixing this. Should we need help with code review and/or testing, I'll comment as much here.

Visual editor impacted
I've updated the task description to mention that the visual editor is impacted by this issue as well.

vector-body is not the same as mw-body-content. The former applies to all elements in the content area, which includes categories, and subtitles and the latter is a standardized class that should mean the same across all skins. I wouldn't advise mw-body as this is not standard at current time and might change in future.

I can advise what's the most future-proof thing to do here, and what to do to fix it, but I'm not too familiar with the 2017 wikitext editor and what is doing (I'm only aware of 2011). How do I enable it? Can you please add some replication steps?

I don't fully understand why you need the skin name in the class name? This seems like a strange architecture to me. We only load one skin at a time, and the class names should just describe their purpose. A skin prefix in the name make sense if the feature was unique to that skin, but here we are talking about paragraph spacing, which is universal. If styles specific to a skin need to be put in a shared CSS file (which itself is probably bad practice) then the body class skin-vector can be used for context.

I don't fully understand why you need the skin name in the class name? This seems like a strange architecture to me.

It's just a prefix. We started using the skin name as a prefix similar to how we do with extensions e.g mw-mf- and ve-. The main reason for this was there was a pattern of skins using mw- prefixed classes to give the impression that these classes were universal to all skins. This was confusing as many mw- prefixed classes are universal to all skins e.g. mw-indicator, mw-body-content. We've been trying to put the true mw- prefixed classes in the core skins files so they can be more relied upon.

Unfortunately for backwards compatibility, there are still a lot of mw- prefixed classes in circulation in skins that are not standardized.

In this case 'vector-body' is an unstable element unique to Vector, that can change at any time.

Hope that makes sense?

A skin prefix in the name make sense if the feature was unique to that skin, but here we are talking about paragraph spacing, which is universal.

If there is a need for a skin specific body class, that class should only be used to style the skin specific feature, e.g. one could have vector-sticky-header as that is a feature unique to that skin, but one should not use that class for style rules that are common across skins, so more specifically, I don't understand why we need .vector-body p rules, which are the cause of this bug.

I think we're getting confused here. There is no skin specific body class used here or needed. My understanding about this bug is that we have a rule that controls font-size on an element in the page that has the class vector-body:

.vector-body {
    font-size: 0.875em;
    font-size: calc(1em * 0.875);
}

This node has multiple children that need to inherit the font-size rule. We also have gadgets to consider:

Screen Shot 2021-10-19 at 8.29.39 AM.png (196×1 px, 64 KB)

VisualEditor tries to simulate this element and uses a class mw-body which it presumably assumed to be a div present in all skins, given the mw- prefix. However the mw-body class is not a universal class, so when Vector used vector-body instead it broke. We are working towards either removing mw- prefixed classes or making them universal when they are not universal to skin, but that's going to take a while.

The most generic solution I can suggest is:
https://phabricator.wikimedia.org/T287733#7325676

A skin specific fix would look like this:

if ( skin === 'vector' ) {
  $( '.mw-body' ).addClass( ' vector-body' )
}

So as far as I understand it, the only decision here is to choose one of the above solutions, and we can patch this. My understanding of the code is limited however so I would need to be given a hint about where to put this code.

It should be possible to apply content-area-like styles by adding a non-skin-specific classes (mw-body-content, mw-parser-output). Aside from VE's wikitext preview dialog, any other extension or gadget that wants to display content in a popup that isn't a child of the content area would need to use this feature. This informal API existed until the introduction of vector-body.

vector-body doesn't just set root font sizes, it also sets line heights and margins for headings, paragraphs etc. Those styles should be controlled by a skin-agnostic class like they used to be.

Copying the class list is not an acceptable solution either, as the main content area my have classes added to it in future (or in other skins) that wouldn't apply to a separate content area.

It seems like we are just repeating the discussion that was had on T279388. Timo and others repeatedly raised very valid concerns which were just dismissed as "out of scope".

Jdlrobson removed a subscriber: Jdlrobson.

Informal contracts have been a huge time suck in the desktop improvements project, delaying most of our roll outs by 2-4 weeks. The team has been trying to rely less on these contracts so that less UI regressions occur on the long term and so that we rely less on the institutional knowledge inside certain individual's heads about "how things are supposed to work".

I've suggested two fixes here, neither of which is controversial.

If we are unhappy with the status quo I suggest we get together and talk about that separately so that we do a better job of understanding pain points from each team's perspective, but let's focus on the bug at hand here.

I've suggested two fixes here, neither of which is controversial.

The first fix requires separate code for each skin so isn't workable.

The second has problems blindly copying all classes as I outlined in my previous comment.

Informal contracts have been a huge time suck in the desktop improvements project

I appreciate this is one of the major complexities of the project, but that doesn't mean they can just be removed, especially without a reasonable alternative.

Change 736861 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Apply typography rules to mw-body-content elements outside vector-body

https://gerrit.wikimedia.org/r/736861

@Esanders would the above patch fix this for you? Happy to take on the technical debt if so. The use of the vector-body class is mostly to avoid breaking certain gadgets.

Test wiki created on Patch Demo by ESanders (WMF) using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/4320a60872/w/

Change 736861 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Apply typography rules to `.mw-body-content` elements outside `.vector-body`

https://gerrit.wikimedia.org/r/736861

I hate to bear the bad news, but this is even worse now after the patch, the text size has also shrunk.

Editor previewRead mode
image.png (2×3 px, 1 MB)
image.png (2×3 px, 1 MB)

(testing on https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein)

I think this change has also caused a global change in font size.

Before this change (commit 2ba2af97cfc55240b25ba0abc5caba87447c2cc4):

before_font_size.png (397×528 px, 33 KB)

After this change (commit da832cc53d2cb009a618182c6c7a85aea3e7fb6b):

after_font_size.png (381×571 px, 38 KB)

Seen on beta and on my local.

I think this change has also caused a global change in font size.

What browser are you using @dom_walden ? Can you replicate this at https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?safemode=1 ?

I hate to bear the bad news, but this is even worse now after the patch, the text size has also shrunk.

@matmarex it looks like this rule is to blame:

.ve-ui-overlay-global {
    font-size: 0.875em;
}

which is what I badly communicated about - this needs Vector change AND a VisualEditor change.

If doing that is complicated, we can revert the Vector change to work out a better approach.

I think this change has also caused a global change in font size.

What browser are you using @dom_walden ? Can you replicate this at https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?safemode=1 ?

I can. Firefox 78.15.0esr from Debian bookworm.

I think this change has also caused a global change in font size.

What browser are you using @dom_walden ? Can you replicate this at https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?safemode=1 ?

I can. Firefox 78.15.0esr from Debian bookworm.

I'm feeling a bit stupid now :).
Can you expand on what you are seeing with regards to font size being wrong? font-size appears to be 14px for me, which is what is in production.
Also is this the legacy Vector (https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?safemode=1&useskinversion=1) or the modern Vector (https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?safemode=1&useskinversion=2) ?

Do you know what CSS rule is causing the problem you are seeing? I'm not sure how https://gerrit.wikimedia.org/r/c/736861 could have impacted the global CSS so there's possible something I don't understand about how the not selector is behaving.

I'm sadly not at all sure what specific rule is causing the issue (or if it is even caused by this task), but the text seems to be much larger (and dev tools reveal that "computed" font-size is 16px compared to 14px in production) and the gap between lines seems to be much smaller

Screenshot from 2021-11-10 19-20-08.png (346×520 px, 55 KB)

Screenshot from 2021-11-10 19-23-41.png (378×552 px, 89 KB)

compared to a page on the production enwiki:

Screenshot from 2021-11-10 19-23-16.png (378×578 px, 75 KB)

I can see why. We used a CSS selector .mw-body-content:not(.vector-body .mw-body-content), .vector-body, but this is not supported until Firefox 84 (according to https://caniuse.com/css-not-sel-list). In CSS3, :not() can only take a simple selector (https://www.w3.org/TR/selectors-3/#negation) – e.g. a single class selector. It only allows complex selectors like .vector-body .mw-body-content in the CSS4 spec.

I see, so presumably that not query is only acting on the vector-body class and has become truthy resulting in the rules applying twice?
e.g. `.mw-body-content:not(.vector-body .mw-body-content) is interpreted as .mw-body-content:not(.vector-body) TIL

I've reverted the patch. @Majavah can you confirm that addresses the issue?

Seems like we dont want to use the not selector here and use the ve specific selector from patchset 1 so I'll prepare that again, but I also need that VisualEditor patch prepared too.

I see, so presumably that not query is only acting on the vector-body class and has become truthy resulting in the rules applying twice?
e.g. `.mw-body-content:not(.vector-body .mw-body-content) is interpreted as .mw-body-content:not(.vector-body) TIL

No, it causes the selector to be invalid, and all comma-separated rules to be ignored. https://www.w3.org/TR/selectors-3/#grouping "If just one of these selectors were invalid, the entire group of selectors would be invalid."

Can we use #mw-content-text as a temporary workaround?

Change 738441 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Fix font size in editor preview

https://gerrit.wikimedia.org/r/738441

Change 738442 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/VisualEditor@master] Adjust font style/content hierarchy for Vector

https://gerrit.wikimedia.org/r/738442

Change 738442 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Fix specificity for mw-body-content in VE overlay

https://gerrit.wikimedia.org/r/738442

Change 738441 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Fix font size in editor preview

https://gerrit.wikimedia.org/r/738441

Change 738516 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Increase specificity of source mode paragraph margin rule

https://gerrit.wikimedia.org/r/738516

Change 738937 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/skins/Vector@master] Revert \"Fix font size in editor preview\"

https://gerrit.wikimedia.org/r/738937

Change 738516 abandoned by Bartosz Dziewoński:

[VisualEditor/VisualEditor@master] Increase specificity of source mode paragraph margin rule

Reason:

Not needed right now because the original patch is being reverted, we can restore it when we know where we stand.

https://gerrit.wikimedia.org/r/738516

Reverted because this change caused T295712: Font-size in VE too small, which is also a worse problem.


It also caused a problem with paragraph margins in the NWE, which we were going to fix with https://gerrit.wikimedia.org/r/738516, but I'm not merging that for now given that we're reverting the other patch.

image.png (2×3 px, 693 KB)

Change 738937 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Revert \"Fix font size in editor preview\"

https://gerrit.wikimedia.org/r/738937