Page MenuHomePhabricator

VisualEditor: saveOptions.summary is not updated if the summary is inserted using a tool (InputWidget relies on 'change' events rather than checking the value)
Closed, DeclinedPublic

Description

The Hebrew Wikipedia, as well as a few others, have a gadget for canned edit summaries:
https://he.wikipedia.org/wiki/MediaWiki:Gadget-Summarieslist.js

If the forceeditsummary user preference is enabled and the user uses this gadget to insert a summary, the warning about an empty edit summary is issued, even though it shouldn't.

I suspect that the problem is in the VE code.
As far as I can see, this preference is checked in
modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.js on line 867
and the summary values are not what I expect them to be.

(See also Bug 52085.)


Version: unspecified
Severity: normal

Details

Reference
bz69749

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:43 AM
bzimport set Reference to bz69749.

This gets a bit weirder for me. It seems that with the gadget that summaries aren't saved at all, even if I type something manually in the textbox, and this behavior is inconsistent:

If I add automatic summaries from the buttons and save, the summary isn't saved.
If I add automatic summaries from the buttons and then add a text, they are saved as a summary.
If I add a summary by hand, it isn't saved.

I'm not sure what would cause this to happen; the gadget fills in automatic text by adding into the textbox.val() which should make this work.

Got it.

The issue was a mixup between dealing directly with the textarea object through jQuery (which updates .val()) and dealing with VE's OOUI elements (which update through .setValue() )

I've created a fix to the gadget, but I don't have the permissions to edit it. I've asked Eran to take a look and implement the fix.

Fixed by triggering manually input event.

However oojs-ui should use the widget value to get must up to date value (e.g protype.getValue( return this.widget.val(); ), and not to be based on some cached variable.
If it does something smart with the value it can be check that if this.value !=this.widget.val() then it should trigger the function for updating this.value, before actually return the widget value.

Created attachment 16237
Fix for oojs-ui

Attaching a proposed patch for oojs-ui.

Attached:

The problem is actually to do with the fact that the browser isn't emitting events when it should. OOjs UI is listening for the change event.

Just use:

$input.val( newValue ).trigger( 'change' );

@Trevor Parscal, I already fixed it with triggering input event. However it should be fixed in oojs-ui itself for later cases.
The browser should not emit event in this case (see RFC: http://www.w3.org/TR/html401/interact/scripts.html#adef-onchange), and even if the spec was the other way - oojs-ui can't blame browsers...

The problem is that we really should be caching the value; it's done to protect the behavior against a lot of event triggering and browser mishaps that otherwise would make interaction with inputs pretty horrible.

We can't have ooui deal with the jQuery element directly; if you notice, htere's also a SetTimeout() method inside 'setValue' -- that's on purpose to make sure that if the user types quickly we don't emit crazy amount of events. Changing ooui to deal with the jQuery element's value directly will remove a lot of its intended functionality.

This is onEdit function (with setTimeout) that protect against browsers mishaps if I understand correctly.

BTW - don't scar to change getValue, as it is used only in ~35 places in the code ;)

There are actually several reasons for the setTimeout, but the real problem here is that getValue compensating for not noticing the value changed previously causes other issues.

The OO.ui.TextInputWidget has a "change" event, which is depended on by other systems. If we just make getValue compensate for not noticing the value changed previously, then systems depending the aggregated change event will behave incorrectly. The best way to handle this is to either trigger the event or come up with a way to access the input. We could, for instance, leave references to widgets in the elements using jQuery.data().

matmarex renamed this task from VisualEditor: saveOptions.summary is not updated if the summary is inserted using a tool to VisualEditor: saveOptions.summary is not updated if the summary is inserted using a tool (InputWidget relies on 'change' events rather than checking the value).Jan 11 2015, 1:38 PM
matmarex added a project: OOUI.
matmarex set Security to None.

I think this should be closed as invalid. Like Trevor said above, the script should call .trigger( 'change' ) on the <input/> node.

We could change #getValue to actually get the .value of the <input/> every time (like in the proposed patch), rather than relying on the cached one, but any scripts interacting with input fields must trigger the 'change' event anyway; there's more machinery that often depends on this – in this particular case, the script that limits the length of edit summary to 255 bytes.

It is a bit weird that OOUI widgets rely on events only to update their internal state, but that's intentional per Trevor's design and I think the design is sensible. It can result in the state of the widget becoming inconsistent with the state of the DOM node when other scripts modify it without triggering events, but on the other hand it ensures that the state of the widget itself is internally consistent.

At least it should be documented properly.
Usually the UI should be consistent with the DOM.

Documented how/where? I'd be happy to if you come up with a sensible way to do it. :) (This isn't really a supported use-case for OOUI, but it's useful and people are going to do it anyway, so documenting something would make sense.)

Just to be clear, when you modify .value from JavaScript (or use jQuery's .val(), etc.), there's literally no way for VisualEditor or any other script to pick up the changes in real-time (which we need for things like validation of the inputted values), unless you trigger a 'change' event or something like that (well, short of polling for changes every 100 ms or so, but let's not do that please). That's just how this works, whether we like it or not.

Sync of the value to the DOM can be done in lazy way on the getValue() (so timer polling isn't really needed...)

What is the correct policy to handle DOM value not equal to the OOUI widgets? Don't know but here are some possible options

  • Trigger change event - this is what I suggested above. This is WYSIWYG - you see X in DOM, X would be the value in the widget.
  • Raise exception - If you fear the above is too dangerous, and this moves the responsibility to the script writter but let him know
  • log.warning(...) - If OOUI lost DOM synchronization too often to raise exceptions. If this is the case then OOUI is probably broken.

Hmm. That is actually a very good point. Talked with @TrevorParscal and you have us both convinced.

Let's file a separate task for this (filed T86868), I'll adapt your patch (

from T71749#745592) for other InputWidgets.