Page MenuHomePhabricator

Adding/removing to watchlist when new user causes window to reopen
Closed, ResolvedPublic

Description

Visit http://localhost:8888/w/index.php?title=Headings&article_action=add_to_collection&mobileaction=beta
Add/remove the page from your Watchlist
The content overlay will confusingly reopen
Only happens for Watchlist...

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to In Analysis on the Gather Sprint Enwiki board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 10 2015, 7:12 PM
Jdlrobson moved this task from Ready for dev to In development on the Gather Sprint Enwiki board.

Change 203842 had a related patch set uploaded (by Jdlrobson):
Prevent auto-reopening of collection overlay

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

Jdlrobson set Security to None.
Jdlrobson moved this task from In Analysis to Code review needed on the Gather Sprint Forward board.

Change 203842 merged by jenkins-bot:
Prevent auto-reopening of collection overlay

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

@Jdlrobson

Seriously though, shouldn't the options object always be the same as this.options? What's going on?

phuedx added a subscriber: phuedx.Apr 16 2015, 12:22 PM

Forgive my stupidity, but I still don't quite understand what's going on here. Context: I came here by way of the byref patch that you submitted.

@phuedx no problems - it's really subtle and I'm confused now trying to explain it. So in render we were basically cloning options and then adding the newly passed parameters.

thus
if options was { a: 1 }

foo.render( { foo: bar } ) would render the template with foo and a
foo.render() would render the template with simply a, regardless of the previous execution and thus current state of the View
but foo.options.foo = bar; foo.render() would render the template with foo and a.
So the change now is that foo.render( { foo: bar } ) overrides existing options.

This became problematic in https://gerrit.wikimedia.org/r/#/c/203842/3/resources/ext.gather.watchstar/CollectionsWatchstar.js as postRender needs to changes the current state of CollectionsWatchstar - it deletes an entry from the options parameter - which is sometimes this.options and sometimes an arbitary object. In L163 of that patch we just call render without any arguments which is why we were hitting this problem

Thanks for the clarification @Jdlrobson.

JKatzWMF closed this task as Resolved.Apr 27 2015, 4:45 AM

@Jdlrobson this is resolved, but appears to have caused a new bug: T97268