Page MenuHomePhabricator

Focussing an <input type="number" /> (e.g. Gallery→Options→Image height) crashes VE on Firefox
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Open an existing Gallery
  2. Go to Options tab
  3. Change or add Image width/ Image height/ Images per row
  4. If you now click on "Apply changes", it will say Something went wrong
  5. If you click on "Cancel", VE crashes

Also, when the cursor is on those fields, the following error appears in the console:

Error: Permission denied to access property "parentNode"
Error: Permission denied to access property "nodeType"

Note: This is not relevant to the recent Gallery changes, since it's already happening on production (en.wiki). But since it's a crash issue, we should probably fix it soon.

Event Timeline

Ryasmeen created this task.Nov 15 2018, 9:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 15 2018, 9:30 PM
Ryasmeen updated the task description. (Show Details)Nov 15 2018, 9:31 PM

I can't reproduce this :( Tested on https://en.wikipedia.org/wiki/Heart#Additional_images. What page are you testing? Can you record it?

Sure! Also, I just found out that this is happening on Firefox only.

Ryasmeen renamed this task from [Regression] Making changes to Gallery>Options> Image width/ Image height/ Images per row crashes VE to [Regression] Making changes to Gallery>Options> Image width/ Image height/ Images per row crashes VE on Firefox.Nov 15 2018, 10:01 PM

Here's a minimal test case for the issue:

<input type="number" />
<script>
document.getElementsByTagName('input')[0].focus();
alert( document.getSelection().anchorNode.tagName );
</script>

To work around this, I think we would have to wrap every access to properties of Selection#anchorNode and #focusNode in a try…catch. At a glance that's ~50 places in the codebase. Certainly possible, but that's a lot of work (and a lot of ugly code to maintain in the future), so I'd like to see if anyone has any better ideas.

Alternatively, we could stop using <input type="number" /> in our code (or NumberInputWidget), and use type="text" instead (TextInputWidget). But we would probably have to add manual validation to each of them if we do that, and the interface is worse, especially on mobile (where number inputs have a specialized keyboard).

The Firefox bug has been reported 2 months ago with no response yet.

matmarex removed matmarex as the assignee of this task.Nov 16 2018, 7:54 PM
matmarex renamed this task from [Regression] Making changes to Gallery>Options> Image width/ Image height/ Images per row crashes VE on Firefox to Focussing an <input type="number" /> (e.g. Gallery→Options→Image height) crashes VE on Firefox.

To work around this, I think we would have to wrap every access to properties of Selection#anchorNode and #focusNode in a try…catch. At a glance that's ~50 places in the codebase.

We usually access the native Selection object via ve.ce.Surface.nativeSelection, so perhaps we could somehow modify the accessor function for those properties.

var proxiedSelection = new Proxy( getSelection, {
  get: function( target, prop ) {
    if (prop === 'anchorNode') {
      try { ... } catch() { ... }
    } else {
      return target[ prop ];
    }
  }
} );

Looks like Proxy breaks methods, but we can use defineProperty and carefully extract the original value. A truly horrendous workaround.

We can also try to avoid any of the CE surface code running when the focus is in a dialog, because it isn't necessary.

Let's say we introduce a getter/wrapper for .anchorNode and .focusNode, either using Proxy, using defineProperty, or just a new method and search-and-replace. But what should this getter actually return when the real anchorNode/focusNode is broken? I have no idea if we might break any assumptions in our other code if we return the "wrong" element, and we can't return the right one. That's why I thought we would have to do it separately for every place where it's used. For the most part, we only do things with anchorNode/focusNode inside our contenteditable node, but I'm not sure if this is always true.

Esanders added a comment.EditedNov 18 2018, 2:33 PM

document.activeElement.parentNode

Change 474533 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Workaround native selection bug for number inputs in Firefox

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

Esanders claimed this task.Nov 18 2018, 2:51 PM
Esanders moved this task from Ready for Pick Up to QA on the VisualEditor (Current work) board.

Change 474533 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Workaround native selection bug for number inputs in Firefox

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

Change 475825 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (6a2233f68)

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

Change 475825 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (6a2233f68)

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

marcella closed this task as Resolved.Dec 14 2018, 4:20 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptDec 14 2018, 4:20 PM