Page MenuHomePhabricator

VisualEditor Mobile: [iOS] abandoning edit takes me to unscrollable page
Closed, ResolvedPublic

Description

Observed on iPad. Steps to repro:

  1. Go into VE edit mode on an article and enter the link inspector
  2. Use browser back to exit the link inspector

Expected: you get a message asking if you want to abandon changes and get taken back to the content if you say yes

Actual: no message about abandoning changes, and you're taken to the top of the page but can't scroll below the fold


Version: unspecified
Severity: normal

Details

Reference
bz69630

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:35 AM
bzimport set Reference to bz69630.

bingle-admin wrote:

Prioritization and scheduling of this bug is tracked on Trello card https://trello.com/c/ttlYWwkt

bingle-admin wrote:

Prioritization and scheduling of this bug is tracked on Trello card https://trello.com/c/kzlyuZku

jgonera wrote:

This actually seems to be a VE/OOUI bug. The problem lies in WindowManager not cleaning up after itself.

I thought it could be easily fixed by adding this.inspectors.clearWindows() in ve.ui.Context.prototype.destroy, but unfortunately that results in:

"Uncaught TypeError: Cannot read property 'getComputedStyle' of undefined "

This happens when WindowManager invokes OO.ui.Window.prototype.hold. I suspect it's because everything in WindowManager is async/delayed (to accommodate for animations), so DOM nodes no longer exist when WindowManager tries to tear windows down.

(In reply to Juliusz Gonera from comment #3)

I thought it could be easily fixed by adding this.inspectors.clearWindows()

I haven't reported this in Bugzilla yet, but I discovered yesterday that clearWindows() is completely broken. It will only clean up the current window (i.e. the one that's currently open), and it won't touch any others.

in ve.ui.Context.prototype.destroy, but unfortunately that results in:

"Uncaught TypeError: Cannot read property 'getComputedStyle' of undefined "

This happens when WindowManager invokes OO.ui.Window.prototype.hold. I
suspect it's because everything in WindowManager is async/delayed (to
accommodate for animations), so DOM nodes no longer exist when WindowManager
tries to tear windows down.

Yeah that sounds likely. Maybe WindowManager needs a destroy function that destroys everything immediately? I also don't really understand why WindowManager not cleaning up is causing problems if the WindowManager's DOM element is removed.

gerritadmin wrote:

Change 155657 had a related patch set uploaded by JGonera:
Remove global overlay classes when destroying MobileSurface

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

gerritadmin wrote:

Change 155657 merged by jenkins-bot:
Remove global overlay classes when destroying MobileSurface

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

gerritadmin wrote:

Change 155668 had a related patch set uploaded by Catrope:
Destroy WindowManagers in Context and Surface destructors

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

gerritadmin wrote:

Change 155668 merged by jenkins-bot:
Destroy WindowManagers in Context and Surface destructors

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

jgonera wrote:

All patches merged, marking as fixed.

Verified the fix in Betalabs and test2

Verified the fix in production