Page MenuHomePhabricator

Massive recalculate style triggered by OO.ui.Widget.setDisabled
Closed, ResolvedPublic1 Story Points

Description

Event Timeline

ori created this task.Feb 13 2015, 12:52 AM
ori assigned this task to matmarex.
ori raised the priority of this task from to Needs Triage.
ori updated the task description. (Show Details)
ori added subscribers: Aklapper, ori, Catrope, Krinkle.

Well, when you disable/enable a tool, there's a bunch of styles that stop/start applying. We can't really work around this. I'm assuming that the problem is that this happens during UI initialization.

But I looked at the stacktrace and stepped through the code, and it look like tools start out enabled, and they only have to be disabled by this code when (simplifying a lot, I haven't even tried to understand the exact conditions) they can't in fact be applied to the current selection (e.g., you can't bold an image). The selection starts out as a cursor at the beginning of the text, unless there's no sane text to put the cursor into; which means that most tools should stay enabled on most pages, and the problematic code never runs. What page were you testing on?

How do I get these pretty profiles? My dev tools profiler doesn't show "Recalculate Style" as a separate item anywhere.

Also, 20 milliseconds is not what I'd call massive…

matmarex changed the task status from Open to Stalled.Feb 16 2015, 7:45 PM
matmarex reassigned this task from matmarex to ori.
matmarex set Security to None.
matmarex added a subscriber: matmarex.
Jdforrester-WMF triaged this task as High priority.Feb 17 2015, 12:17 AM
ori reassigned this task from ori to matmarex.Feb 17 2015, 7:26 PM

What page were you testing on?

http://osmium/wiki/Barack_Obama

To reach it, you'll have to set up a SOCKS proxy to the production cluster. Let me know if you need help with that.

How do I get these pretty profiles?

https://lists.wikimedia.org/pipermail/wikitech-l/2015-February/080800.html

My dev tools profiler doesn't show "Recalculate Style" as a separate item anywhere.

Also, 20 milliseconds is not what I'd call massive…

I think 20ms is massive.

In T89423#1044418, @ori wrote:

What page were you testing on?

http://osmium/wiki/Barack_Obama
To reach it, you'll have to set up a SOCKS proxy to the production cluster. Let me know if you need help with that.

Which presumably would require him to have some sort of existing access to the production cluster?

Indeed, I don't have access to production (and don't really want it). I'll assume that you basically tested on en.wp, dig in and see what happens, but this task doesn't really look actionable to me.

Good news! I have discovered that we accidentally disable and then re-enable pretty much the entire toolbar while the page loads. Getting rid of that should cut down on these pesky setDisabled() calls.

  • ve.init.mw.Target.prototype.onReady
    • this.setupSurface( … ) → ve.init.mw.Target.prototype.setupSurface
      • target.setSurface( surface ) → ve.init.Target.prototype.setSurface
        • this.setupToolbar( surface ) → ve.init.mw.ViewPageTarget.prototype.setupToolbar
          • target.getToolbar().initialize() → ve.ui.Toolbar.prototype.initialize
            • this.updateToolState() – at this point, the surface has no selection (and isn't focused), so the tools can't be applied, so they get disabled
      • target.emit( 'surfaceReady' ) → ve.init.mw.ViewPageTarget.prototype.onSurfaceReady
        • this.getSurface().getView().focus() – since there is no selection yet, one will be created, which emits a contextChange event, which goes to ve.ui.Toolbar.prototype.onContextChange and again this.updateToolState() – there is a selection now, applicable tools get re-enabled

Possibly we could just remove the .updateToolState() call in ve.ui.Toolbar.prototype.initialize (added in 21371ce846f72e3d249557f4a2939b8b2fe2c273 / https://gerrit.wikimedia.org/r/#/c/129806/). But not sure how that would affect non-MW uses of VE, and @Esanders and @Catrope probably had a good reason to add it (it was even cherry-picked to prod back then, sadly no records exist about why that was done).

what is the order of things in core?

Change 191378 had a related patch set uploaded (by Bartosz Dziewoński):
ve.ui.Toolbar: Don't call #updateToolState from #initialize

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

Patch-For-Review

Standalone has the same order – first setSurface() is called, calling Toolbar#initialize, which disables everything because there's no selection (NullSelection); then the surface is focused, emits contextChange, which enables everything (other than tools which don't apply, so basically only table cell and header format tools in most cases).

Just be careful when writing a fix here because the correct initial select in the model is in fact NullSelection. We then attempt to find somewhere to place the cursor initially, but if your document only contains an image, then the selection will stay null and no contextChange will fire. If we just removed the updateToolState form initialize it wouldn't get called at all in this edge case (I think).

matmarex added a comment.EditedFeb 18 2015, 9:54 PM

The worst case is that toolbar icons will not be disabled when they should, until the user clicks the surface themselves. But that's not going to happen anyway because, as far as I can tell, it's completely impossible to get a NullSelection after calling surface.focus() – there's always a slug or regular text node to put the cursor into.

Change 191378 merged by jenkins-bot:
ve.ui.Toolbar: Don't call #updateToolState from #initialize

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

Jdforrester-WMF closed this task as Resolved.Feb 18 2015, 11:10 PM
Jdforrester-WMF moved this task from Bug Fixes to Q4 on the VisualEditor board.
Jdforrester-WMF edited a custom field.
Jdforrester-WMF moved this task from Accepted to Done on the VisualEditor 2014/15 Q3 blockers board.
matmarex reopened this task as Open.Feb 18 2015, 11:12 PM

@ori, I couldn't quite reproduce the flame graphs, so can you recheck with the above patch merged? It shouldn't affect setDisabled itself, but it means that setDisabled will be called a lot less, and hopefully that recalculate was a result of several calls coming together.

ori moved this task from Backlog to Done on the VisualEditor-Performance board.Feb 19 2015, 2:28 AM
Jdforrester-WMF moved this task from Backlog to Reviewing on the OOUI board.Mar 26 2015, 9:00 PM