Page MenuHomePhabricator

wikipage.content is fired twice on RC load with new filters
Closed, ResolvedPublic1 Story Points

Description

How to reproduce:

  1. Go to Beta on preferences and make sure "New filters for edit review" is enabled
  2. Add user JS with custom mw.hook( 'wikipage.content' ) and inject some content to each entry (such as new button/link/whatever)
  3. Open RC page - the content will be injected twice

Reason:

  1. mw.hook( 'wikipage.content' ) is fired once (regular page load)
  2. mw.hook( 'wikipage.content' ) is fired on model change on the filters - this is fine when the user is changing the filters, but less cool if the model was just initialized

This should be fixed to avoid each user script/gadget does a explict check if the content was already passed.

Details

Related Gerrit Patches:

Event Timeline

eranroz created this task.May 15 2017, 4:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 15 2017, 4:34 PM

Good point, but I am running into an issue here. The reason we fire 'wikipage.content' again in ChangesListWrapperWidget is mostly because even on load, we ingest the results into the widget, and then we want to make sure that the EnahncedRecentChanges gets the scripts applied to it after the wikipage.content hook.

I was thinking of adding a class variable "this.initialized" that turns true when FiltersViewModel finishes its initialization and have the hook first check whether the model is initialized first before firing the hook, but that means that if you're on Enhanced Recent Changes view, the first load won't have all the small scripts, like the arrow-key used to expand the sub-lists, etc.

If we could prevent the wikipage.content hook from firing on-load (the regular page load) that would have been ideal - have it wait until we're actually ready -- but that can't be stopped, so we seem to be stuck having to fire it twice.

I think we might need to rethink the strategy of how we "ingest" the initial load of the changes list properly.

By the way - we fire this hook each time with the specific section of the page that was potentially updated. For the search fieldset (with number of results, dates, etc) we also fire this hook but only include that DOM element; for the results set, we fire the hook with strictly the results. The original load fires the event with the entire page -- but that's before the page is ready.

We need to come up with a way to set this up properly so we don't mess gadgets up, but we still let gadget manipulate the actual content on the page after load is complete. I'll try to think about this, but ideas are welcome.

@eranroz Can you maybe give an example of how this affects a gadget? I want to fix this, but right now it's not a good idea to *not* fire one of those events, so I'm trying to see how best to fix the issues that come up.

Examples for gadgets that manipulate the recent changes and may be broken with the new filters

A more particular example is how this is fixed: https://he.wikipedia.org/w/index.php?title=%D7%9E%D7%93%D7%99%D7%94_%D7%95%D7%99%D7%A7%D7%99%3AGadget-QuickRCDiff.js&type=revision&diff=20704209&oldid=19687674 (note - the gadget had to mark the changed content with qRcDiff class otherwise the click handler will be registered twice).

Okay, I have an idea, working on it. Patch will come soon.

Change 356180 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] RCFilters: Only emit wikipage.content once on load

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

@eranroz Okay, this should do it; I've tested it to make sure the hook only runs once on page load (for all elements of the page) and then it will run twice per result-update, but with two DOM elements (once with the change list and once with the fieldset, each at the time where they are fully updated)

This should fix the behavior for gadgets.

For posterity and to share with others who watch this task, copied from the discussion in gerrit:

Patch Set 3:

@Catrope wrote:
But the ranOnce check is in onModelUpdate, not in processContainer,
so it appears to be designed to ignore the first model update
event.

@Mooeypoo:
Actually, while I was going to explain why ranOnce is needed, I realized there's a deeper problem here that may require us to emitting this hook for the content anyways on load.
The issue is that when we normalize the URL, the content updates too, but usually on init that's a no-op, so emitting twice is moot, which is why there's the "ranOnce" check (updating url -> triggers updateChangesList -> triggers the update event and hook, etc)
However, there's a bigger issue here. We are also checking whether to load the state from defaults -- and defaults are not necessarily the same defaults that are already loaded. If the defaults are form a saved query, we actually *do* change the content on load, so we will *have* to update the model *and* the content list -- and emit the hook! -- on load, again, in that case.
I'm switching this commit to WIP and I'll have to get back in and see how I can balance between those cases, so at least we only fire the hook when the content *actually* changed, and not if we "just" took the current content we receive from the backend and digested it into the widget.

So to wrap up, @eranroz there are cases where in initialization of the page the hook will have to be fired again because the content of the results is changed (when we load defaults that are not the wiki/system defaults) -- I will make sure that the hook is only fired for *that* case, though, so that gadgets will still be able to manipulate the DOM and then just manipulate whatever new DOM rather than the same DOM twice.

Catrope added a comment.EditedJun 14 2017, 12:54 AM

Never mind, that duplicate was a false positive. The hook fires twice every time, but with different DOM nodes.

Change 356180 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Don't emit wikipage.content on first load

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