Page MenuHomePhabricator

[Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken, but the problem is rare and intermittent
Closed, ResolvedPublic0 Estimated Story Points

Description

UI for Link inspector, Citation dialog and Media dialog is broken on Beta

Screen Shot 2018-05-24 at 4.43.49 PM.png (560×1 px, 141 KB)

Screen Shot 2018-05-24 at 4.46.42 PM.png (276×461 px, 29 KB)

Screen Shot 2018-05-24 at 4.47.58 PM.png (155×918 px, 16 KB)

Event Timeline

Ryasmeen renamed this task from [Regression pre-wmf.6] Link inspector UI is broken on Beta to [Regression pre-wmf.6] Link inspector/Citation/Media UI is broken on Beta.May 24 2018, 11:48 PM
Ryasmeen updated the task description. (Show Details)

Actually its broken for couple of other dialogs such as Musical notation, Gallery, Chemical formula, Math, Maps, Page Settings, Advanced Settings.

Here are some more screenshots.

Screen Shot 2018-05-24 at 4.57.21 PM.png (460×695 px, 63 KB)

Screen Shot 2018-05-24 at 4.57.15 PM.png (449×681 px, 52 KB)

Screen Shot 2018-05-24 at 4.56.17 PM.png (472×908 px, 191 KB)

Screen Shot 2018-05-24 at 4.55.54 PM.png (467×916 px, 60 KB)

Ryasmeen renamed this task from [Regression pre-wmf.6] Link inspector/Citation/Media UI is broken on Beta to [Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken on Beta.May 25 2018, 12:00 AM
Deskana triaged this task as Unbreak Now! priority.May 25 2018, 10:04 AM
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.

I don't see this broken on beta / locally. Did it get fixed somewhere else?

Your screenshots look as if the styles for the 'oojs-ui-widgets' module were not loaded. All of the visually broken widgets belong to that module (TabSelectWidget, ButtonSelectWidget, OutlineSelectWidget).

And in the recent change fc65ff17d9f132aa332c99f7da44b92f6a372826 (https://gerrit.wikimedia.org/r/#/c/428326/), we split off the styles for this module to a separate one called 'oojs-ui-widgets.styles', which was added as a dependency.

So my theory is:

  1. You opened the page before that change was deployed. The module registry (in 'startup' module) was loaded with the old data (without a dependency from 'oojs-ui-widgets' on 'oojs-ui-widgets.styles').
  2. You waited for a few minutes. In the meantime, the change fc65ff17d9f132aa332c99f7da44b92f6a372826 was deployed to Beta Cluster.
  3. You opened VisualEditor, which would load the module 'oojs-ui-widgets' (among many others).
  4. Module 'oojs-ui-widgets' was loaded, but since new code was already deployed, it had no styles associated with it; and because we had the old module registry, the code did not know it has to load 'oojs-ui-widgets.styles'.

Now, there is a minor hole in this theory, namely that the change was merged a week ago and should have been on Beta Cluster for a long time. But the auto-update mechanism may have been broken (it seemingly breaks often), or you might have had the browser tab opened for a week instead of a few minutes (not impossible).

The same issue can occur in production on Monday when wmf.5 is deployed. It will disappear after just refreshing the page, so I am not sure if it is worth working around.

If we want a work-around to avoid it, we could re-add 'themeStyles' => 'widgets', to the definition of 'oojs-ui-widgets' (and remove it again in the future). But this will cause the styles to be loaded twice for all users not affected by this issue.

Esanders claimed this task.

Let's assume this was some cache weirdness, and re-open if it happens again.

Ryasmeen reopened this task as Open.EditedMay 31 2018, 9:42 PM

Ok this happened again just now on Beta cluster. And I didn't have the page that I am editing open for a week :)
It was all fine a minute ago.

Screen Shot 2018-05-31 at 2.39.43 PM.png (299×442 px, 23 KB)

Screen Shot 2018-05-31 at 2.39.31 PM.png (452×702 px, 54 KB)

However, clearing the cache does resolve it.

If clearing the cache resolves it, then the most likely culprits are either that it was a transient issue or that you somehow still had something cached that got cleared out. Either way, I can't reproduce this, so I have to assume it was fixed.

This issue periodically keeps re-surfacing on Beta cluster even though it temporarily fixes itself after clearing the cache every time. So I assume there is something going on repeatedly for this to happen instead of being a side effect of that change mentioned above? So far it's been on Beta cluster only for me though, not in production.

@Ryasmeen I don't know why that would be happening D:

Deskana lowered the priority of this task from Unbreak Now! to Medium.Jun 19 2018, 10:39 AM
Deskana added a subscriber: Trizek-WMF.

I'm not calling this "Unbreak Now!" any more because it's an incredibly transient issue, but considering @Trizek-WMF can now also transiently reproduce this, it needs looking in to again.

I suspect that recent changes in OOUI have fixed this problem... but if they've not, let me know.

I'm still seeing this in prod about once or twice a week. Been hesitant as I wasn't able to consistently reproduce it after closing and re-opening the browser, but given it happens at least once a day or so for the past week, re-opening now. It affects both the link inspector and the citation inspector.

This is from a few minutes ago in production, en.wikipedia.org, latest Chrome:

Screen Shot 2018-07-05 at 21.41.19.png (1×2 px, 829 KB)

Thanks @Krinkle!

We seem to be at a loss as to what is causing this. :-(

Deskana renamed this task from [Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken on Beta to [Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken, but the problem is rare and intermittent.Jul 6 2018, 9:43 AM

Screen Shot 2018-05-31 at 2.39.31 PM.png (452×702 px, 54 KB)

I saw something similar to this on a test server, where ButtonGroupWidgets were broken up. Inspecting the CSS it looked like the the display:inline-block rule was being applied before the display:block rule, so this could be a CSS load order issue in ResourceLoader?

To investigate I need to know which style rules control this behaviour, and what modules are they in.

Meanwhile, worth checking:

  • If these two rules are in separate modules, are they all loaded at once? If so, check that the one meant to apply last depends on the one meant to apply earlier.
  • If they're not loaded together, perhaps they are triggered by lazy-loaded features? If so, check which actions trigger the lazy-load and whether the bug is tied to a particular order of user actions. This might identify a missing dependency relation between two modules.

I have some ideas, I have yet to test them.

Thanks for the hints @Esanders @Krinkle. I figured out what causes the issue.

The missing dependency is from 'oojs-ui-widgets.styles' on 'oojs-ui-core.styles'. 'widgets.styles' must be loaded after 'core.styles'.

Both of them are style-only modules. When loaded from the styles queue (using addModuleStyles()), they always load in the correct order, because the queue is sorted alphabetically.

When they are loaded from the general queue (e.g. as a result of mw.loader.load('oojs-ui-widgets')), the order is not enforced. It seems that most of the time it works out fine (maybe it's also alphabetical), but if 'widgets.styles' is explicitly loaded first (or loaded from the style queue), the order will be wrong.

However, we can't simply add 'dependencies' => 'oojs-ui-core.styles', to 'oojs-ui-widgets.styles' – while that fixes the issue for VE, it prevents the module from being loaded from the styles queue (causes an error like Unexpected general module "oojs-ui-widgets.styles" in styles queue.), and that causes a FOUC on Special:Preferences?ooui=1.

'oojs-ui-widgets.styles was split off from 'oojs-ui-widgets' specifically because we wanted to load the styles even without JS on Special:Preferences and avoid the FOUC: rMWfc65ff17d9f1: Special:Preferences: Construct fake tabs to avoid FOUC / https://gerrit.wikimedia.org/r/c/mediawiki/core/+/428326.

Here's a small snippet to run in the console for easy reproduction:

if ( mw.loader.getState( 'oojs-ui-core.styles' ) !== 'registered' ) {
	alert( "oojs-ui-core already loaded by an extension or gadget, " +
		"can't reproduce T195544 until you find what loads it and disable it" );
} else {
	mw.loader.using( 'oojs-ui-widgets.styles' ).then( function () {
		mw.loader.using( 'oojs-ui-core.styles' ).then( function () {
			// Load VisualEditor
			$( '#ca-ve-edit a' ).trigger( { type: 'click', which: 1 } );
		} );
	} );
}

I don't know how to best fix it. My only idea right now is to merge 'oojs-ui-widgets.styles' back into 'oojs-ui-widgets' (undoing changes from 428326), and copy-paste the CSS we need to avoid FOUC on Special:Preferences to some separate module that is loaded only on Special:Preferences.

and copy-paste the CSS we need to avoid FOUC on Special:Preferences to some separate module that is loaded only on Special:Preferences.

Could we just all OOUI styles and make it more efficient in a follow-up?

Just came across this in Chrome/MacOS in Cite dialog. Normal reload made it disappear.

Could we just [load] all OOUI styles and make it more efficient in a follow-up?

Yeah, that should be fine.

Change 453286 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Load styles in 'oojs-ui-widgets' again rather than a separate module

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

Is there a task about supporting dependencies for style-only modules?

Is there a task about supporting dependencies for style-only modules?

Prior work:

In short, not supported by current design. This was a trade-off between on the one hand: flexibility, and on the other hand scalability and developer usability. The latter won in this case.

An implementation of "styles dependencies" could only exist in the current system as a way to help guide/enforce the current procedure. Which is, to list out the logical dependencies explicitly in a flat list, with the knowledge that 1) changes to files immediately affect all cached pages referencing the module, and 2) changes to the flat list only propagate slowly as the HTML/CDN cache churns. This requires changes to the dependency tree to be rare and carefully executed, usually by adding a no-op cycle change before/after.

When both of these modules are loaded as style-only modules (using addModuleStyles() in PHP), everything works fine [..]. But when they are loaded as dependencies from JS code, the order is unspecified.

Ah, perhaps you were referring to the ability to depend "on" a styles-only module, as opposed to dependencies between styles-only modules. Because it sounds like the styles-only (PHP) case is already working for you.

The answer to that is, Yes, you can! In 2015 I implemented this as way to simplify and optimise OOUI handling. See T92459 and T87871. They can be depended on without risking that they load twice.

When both of these modules are loaded as style-only modules (using addModuleStyles() in PHP), everything works fine [..]. But when they are loaded as dependencies from JS code, the order is unspecified.

Ah, perhaps you were referring to the ability to depend "on" a styles-only module, as opposed to dependencies between styles-only modules. Because it sounds like the styles-only (PHP) case is already working for you.

The answer to that is, Yes, you can! In 2015 I implemented this as way to simplify and optimise OOUI handling. See T92459 and T87871. They can be depended on without risking that they load twice.

Not sure, @matmarex: does this help?

Change 453286 merged by jenkins-bot:
[mediawiki/core@master] Load styles in 'oojs-ui-widgets' again rather than a separate module

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

Ah, perhaps you were referring to the ability to depend "on" a styles-only module, as opposed to dependencies between styles-only modules. Because it sounds like the styles-only (PHP) case is already working for you.

The answer to that is, Yes, you can! In 2015 I implemented this as way to simplify and optimise OOUI handling. See T92459 and T87871. They can be depended on without risking that they load twice.

Not sure, @matmarex: does this help?

No, in this case, we would have needed to depend from a style module on a style module. ResourceLoader currently can only handle dependencies from general modules on general or style modules.

matmarex removed a project: Patch-For-Review.

The patch above should fix this issue for VE and for watchlist (T197622), and will be deployed to Wikimedia wikis this week, per the usual schedule.

We might want to file a tech debt task about avoiding the double-loading of styles on Special:Preferences somehow. It might be possible to do something clever with dependencies and a skipFunction.

Not seeing this issue more, so calling it verified.