Page MenuHomePhabricator

[Regression wmf.23] Drop down for menu options are overlapping with Toolbar when scrolling up
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Open VE and make an edit.
  2. Save the page.
  3. Re-open the page with VE.
  4. Open any drop down such as Insert/Paragraph styling etc.
  5. Scroll down "slowly" while the drop down is open.
  6. Scroll back up "slowly" to the beginning of the page.

The drop down is overlapping with the toolbar.

Event Timeline

Ryasmeen created this task.Oct 1 2018, 8:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 1 2018, 8:32 PM
Ryasmeen updated the task description. (Show Details)Oct 1 2018, 8:33 PM

Can't reproduce.

Ryasmeen updated the task description. (Show Details)Oct 1 2018, 8:36 PM

So that was chrome:

and here's how it is in Firefox:

Deskana triaged this task as High priority.Oct 3 2018, 10:03 AM
Deskana moved this task from To Triage to Up next on the VisualEditor board.
JTannerWMF moved this task from Up next to Current work on the VisualEditor board.Oct 3 2018, 4:49 PM
JTannerWMF edited projects, added VisualEditor (Current work); removed VisualEditor.
Deskana changed the task status from Open to Stalled.Oct 3 2018, 4:50 PM
Deskana moved this task from Incoming to QA on the VisualEditor (Current work) board.

@Ryasmeen This is high priority, but people are having trouble reproducing it, so this is stalled on getting more detailed reproduction steps for now.

Ryasmeen updated the task description. (Show Details)Oct 3 2018, 8:46 PM

@Deskana: It's pretty straightforward to reproduce, there is also a screen capture attached. I changed the repro steps a bit to indicate it needs to be scrolled up/down slowly :)

@matmarex: Were you able to reproduce the jittering effect on Firefox?

Also, sometimes the drop down menu is completely detached from the toolbar on Chrome while scrolling back up

Maybe try on this page: https://en.wikipedia.beta.wmflabs.org/wiki/User:RYasmeen_(WMF)/sandbox?veaction=edit

Ryasmeen added a comment.EditedOct 4 2018, 5:13 AM

@Deskana: I just remembered the issue you filed couple weeks ago: T203720 .This seems a bit like that one. Atleast that detached drop-down part. Although the steps to replicate you mentioned did not involve any scrolling.

@Deskana: I just remembered the issue you filed couple weeks ago: T203720 .This seems a bit like that one. Atleast that detached drop-down part. Although the steps to replicate you mentioned did not involve any scrolling.

Yeah, I just thought of that this morning, in particular that it only seemed to be affecting me.

I still can't reproduce the problem even following your exact steps. :-/

Ryasmeen added a comment.EditedOct 4 2018, 8:56 PM

@Deskana: I think I found the missing piece, it was not happening for me this morning as well. But then it started happening when I saved my edit, reopened the editor and then followed the steps to reproduce. Could you/@matmarex try one more time following the new steps?

Ryasmeen updated the task description. (Show Details)Oct 4 2018, 8:57 PM

Thanks, that helps. I can see some erratic behavior when scrolling now.


I'll investigate it.

The issue occurs because two handlers for the Window 'scroll' event are executed in the wrong order:

  • A: ve.init.Target#onContainerScroll – Changes the toolbar between "floating" and normal mode
  • B: OO.ui.mixin.FloatableElement#position – Positions the dropdown relative to the toolbar

We expect A to run before B, so that the dropdown is positioned relative to the toolbar in the right mode. When B runs before A, this bug happens.

Event handlers (registered for the same event on the same node) are executed in the same order in which they were registered. But our code should ensure they are registered in the right order:

  • A is registered in ve.init.Target#bindHandlers (called from ve.init.Target constructor)
  • B is registered in OO.ui.mixin.FloatableElement#togglePositioning (called when the dropdown is opened)

I verified that this happens in the expected order. So, how are the event handlers executed in the wrong order?

Event handler A is registered by directly calling addEventListener(), while event B is registered via jQuery (which eventually calls addEventListener()). Some jQuery monkey business seemed like the only possible cause of the problem, so I went to read the source code.

The interesting part happens in jQuery.event.add: on lines 5030-5042, it will only call addEventListener() for the first event handler (for the same event on the same node); any additional event handlers are just added to an internal queue. As a result of this, all event handlers registered via jQuery will actually execute together when the first of them is supposed to execute.

This is new to me. It might be relevant in some more situations, since we mix the two ways of registering events quite liberally in our code.

For example, in this code, handlers will execute in the order A, C, B. Demo: http://jsfiddle.net/1uhaj623/

$( window ).on( 'scroll', A );
window.addEventListener( 'scroll', B, false );
$( window ).on( 'scroll', C );

[There is one more caveat – jQuery calls removeEventListener() when the last event handler it knows of is removed, so if our teardown code removed all the Window 'scroll' event handlers, the issue wouldn't occur. But it turns out that something else registers its own handler (mw.notification#updateAreaMode), which is never removed.]

Change 465811 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.init.Target: Re-position toolgroup popups after floating/unfloating toolbar

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

matmarex changed the task status from Stalled to Open.Oct 11 2018, 1:20 AM

Change 465811 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.init.Target: Re-position toolgroup popups after floating/unfloating toolbar

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

Change 466699 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (0dff6d406)

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

Change 466699 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (0dff6d406)

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

Yay! So the issue with the dropdown overlapping the toolbar is fixed! However, the jittering of the dropdown on Firefox is still happening, so it's probably a separate issue. Want me to file another task for it @matmarex?

Hm, yeah, please do.

Ryasmeen edited projects, added Verified; removed Editing QA.Nov 9 2018, 10:45 PM
Ryasmeen moved this task from QA to Product owner review on the VisualEditor (Current work) board.
marcella closed this task as Resolved.Nov 19 2018, 7:26 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptNov 19 2018, 7:26 PM