Page MenuHomePhabricator

CTA, reference and Red link Drawers should be removed from DOM after close
Closed, ResolvedPublic5 Estimated Story Points

Description

Note: the associated epic T214647 should be read first before taking on this task!

The Drawer hide event removes a few CSS classes from the Drawer’s DOM element, but it never removes the Drawer’s actual HTML, and it also doesn’t remove the CSS classes on the HTML element.

Steps to reproduce

Open a reference, close a reference. Open the web inspector.

Expected results

The reference drawer element should no longer exist in the DOM

Actual results

The reference is still in the DOM, just below the <body/> closing tag, but hidden.

This leads to bugs like T211691 where the .has-drawer class on the HTML element clashes with other classes.

However, One Does Not Simply remove a class from the HTML element. The Drawer features a fade-in and fade-out animation, and to preserve this animation, the element must be removed after the animation finishes. This means adding a timer to the hide method (which means we should also guard against multiple calls to that timer). A timer is also prone to breakage if the CSS animation duration changes. It should be determined if we can somehow keep the CSS animation duration and the duration of the timer in sync (maybe with window.getComputedStyle)?

Bonus round
The Drawer is animated with a .drawer-visible class on the HTML element. This can probably be replaced with a CSS animation on the drawer itself, which will be executed when the Drawer is appended to the DOM. A CSS class would still be required to reverse the animation before the element is detached from the DOM., but at least this effect would be contained entirely in a keyframe animation, instead of in opacity, display and transition css rules.

AC

  • [ x] All Drawer html and CSS classes should be removed from the DOM after a drawer closes.

QA Steps

  • ReferenceDrawer: Click on a reference. Close the reference. Is the Drawer element removed from the DOM? Doe s it still fade-in and out correctly
  • Repeat for CTA Drawers: click the watch star while anonymously
  • Repeat for RedLinkDrawer: Find a page with a red link. Click the red link.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 17 2019, 2:33 PM
Jdlrobson added a subscriber: Jdlrobson.

The reference drawer element should no longer exist in the DOM

I'm not 100% sure about this.
I think the expectation right now is that only one Drawer can be displayed at any given time.

This leads to bugs like T211691 where the .has-drawer class on the HTML element clashes with other classes.

I don't think that being in the DOM is the problem here, but that when hidden it doesn't cleanup after itself.

Will chew on this one a little more. Clearly the status quo is not good enough, but I'm not sure switching out add/remove classes gives us anything, as we add the classes to support animations and to apply opacity to other elements.

Jdlrobson updated the task description. (Show Details)Jan 24 2019, 10:02 PM
Jdlrobson triaged this task as Medium priority.Jan 24 2019, 10:16 PM

Change 553162 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drawer is passed onShow function

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

Change 553163 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Remove references from the DOM on close and manage body classes

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

Change 553162 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drawer is passed onShow function

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

Pulling into sprint as this cleaned up the solution to T225213 by fixing the Reference Drawer (when merged: adjust acceptance criteria by ticking resolved boxes, move out of sprint, remove "patch for review")

nray claimed this task.Nov 27 2019, 6:14 PM

Change 553427 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] 🐛 Make Drawers close when shield is clicked

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

nray removed nray as the assignee of this task.Nov 28 2019, 12:05 AM
nray added a subscriber: nray.

Change 553427 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] 🐛 Make Drawers close when shield is clicked

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

ovasileva set the point value for this task to 5.Dec 4 2019, 6:27 PM
nray assigned this task to Jdlrobson.Dec 9 2019, 6:17 PM

@nray this looks done to me? Is there anything remaining or can this go into QA?

Change 553163 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove references from the DOM on close and manage body classes

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

Jdlrobson renamed this task from Drawers should be removed from DOM after close to CTA and Red link Drawers should be removed from DOM after close.Dec 16 2019, 6:12 PM
Jdlrobson updated the task description. (Show Details)

This is fixed for red link drawer but not the other drawers..

Jdlrobson renamed this task from CTA and Red link Drawers should be removed from DOM after close to CTA, reference and Red link Drawers should be removed from DOM after close.Dec 16 2019, 6:12 PM

Change 558252 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] CtaDrawers no longer stay behind in DOM

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

Change 558252 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] CtaDrawers no longer stay behind in DOM

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

Jdlrobson closed this task as Resolved.Jan 7 2020, 10:36 PM
Jdlrobson updated the task description. (Show Details)