Page MenuHomePhabricator

Uncaught TypeError: Cannot read property 'defaultView' of null
Closed, ResolvedPublic

Description

Problem:

  • A uncatched JavaScript error is thrown
  • The popup tooltip does not have the triangle, and does not go away automatically.

Steps to reproduce:

  • Visit a Special:Translate in proofread mode (example)
load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6150 Uncaught TypeError: Cannot read property 'defaultView' of null
    at getStyles (load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6150)
    at curCSS (load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6161)
    at Function.css (load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6756)
    at load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6905
    at jQuery.access (load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:4182)
    at jQuery.fn.init.css (load.php?debug=true&lang=fi&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1ozcoro:6887)
    at Object.OO.ui.Element.static.getClosestScrollableContainer (oojs-ui-core.js?d7ad9:1202)
    at OoUiPopupWidget.OO.ui.mixin.FloatableElement.togglePositioning (oojs-ui-core.js?d7ad9:4202)
    at OoUiPopupWidget.OO.ui.PopupWidget.toggle (oojs-ui-core.js?d7ad9:5038)
    at ext.translate.messagetable.js?7aeec:287

This issue must have started happening quite recently. I would appreciate help figuring out whether something needs to change in Translate or in OOjs UI.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2017, 8:13 AM
matmarex claimed this task.Mar 19 2017, 2:22 PM
matmarex added a subscriber: matmarex.

The root cause is that OO.ui.Element.static.getClosestScrollableContainer( document.body ) throws an error, rather than returning the root scrollable element (OO.ui.Element.static.getRootScrollableElement()).

The immediate cause is probably rGOJUa46af11dd5f3: PopupWidget: Position anchor relative to popup, not popup relative to anchor, which (among other changes) made all PopupWidgets default to this.$element.parent() as their $floatableContainer. For popups attached directly to <body>, that ends up as $floatableContainer, and later we call getClosestScrollableContainer() on that value, resulting in the exception.

Also, fun fact, the error occurs on Chrome but not on Firefox, due to inconsistencies in how they handle the root scrollable element.

I'm actually not sure if getClosestScrollableContainer should return the root for the body, since arguably body doesn't have a scrollable container – it is the ultimate scrollable container. But that would probably be saner than retuning null or throwing exceptions, and even our own code expects that behavior.

Change 343549 had a related patch set uploaded (by Bartosz Dziewoński):
[oojs/ui] Element: Add special case for document root in getClosestScrollableContainer

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

Side note… looks like the way you position the popup no longer works correctly, partially also due to that change, partially due to rGOJUa3caf97b7167: PopupWidget: Make popups able to actually pop *up*, as well as sideways. But I'm not really sure if this should be considered a regression in OOjs UI or a pre-existing bug in how you do positioning that was now exposed.

For reference, the current code is:

offset = $icon.offset();
tooltip.$element.css( {
	top: offset.top + $icon.outerHeight() + 5,
	left: offset.left + $icon.outerWidth() - 20
} ).appendTo( 'body' );
tooltip.toggleClipping( false ).toggle( true );

There are two ways you could fix it:

  1. Disable the built-in positioning and position manually after you display the popup:
offset = $icon.offset();
tooltip.$element.appendTo( 'body' );
tooltip.toggle( true ).toggleClipping( false ).togglePositioning( false );
tooltip.$element.css( {
	top: offset.top + $icon.outerHeight() + 5,
	left: offset.left + $icon.outerWidth() - tooltip.$element.width() / 2
} );
tooltip.$anchor.css( 'margin-left', '50%' );
  1. Instruct the popup to position itself relative to the checkbox icon:
tooltip.$element.appendTo( 'body' );
tooltip.setFloatableContainer( $icon );
tooltip.toggle( true ).toggleClipping( false );
matmarex edited subscribers, added: Catrope; removed: Nikerabbit.Mar 19 2017, 3:43 PM
matmarex added a subscriber: Nikerabbit.
Amire80 triaged this task as High priority.Mar 24 2017, 7:24 AM
Volker_E moved this task from Backlog to Doing on the OOUI board.Mar 27 2017, 1:20 AM

Change 343549 merged by jenkins-bot:
[oojs/ui@master] Element: Add special case for document root in getClosestScrollableContainer

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

Volker_E moved this task from Doing to OOjs-UI-0.20.1 on the OOUI board.Mar 30 2017, 11:06 PM
Volker_E edited projects, added OOUI (OOjs-UI-0.20.1); removed OOUI.
Volker_E removed a project: Patch-For-Review.
Volker_E removed a subscriber: gerritbot.

On 0.20.2 the positioning is completely broken (this is without changes in Translate):

I have to scroll down to even see it.

Same effect on ULS tooltip (T161203).

Yeah, like I wrote above (T160852#3112935), you current code conflicts with the new built-in positioning. Depending on your perspective, either your code relied on unspecified behavior of OOjs UI, or we made an accidental unaccounced breaking change. But either way, at this point it should probably be fixed in Translate – feel free to copy-paste one of the snippets I gave in my last comment.

From the OOjs UI point of view, this issue is fixed (after my patch, no exceptions are thrown). I'll split off a separate task for Translate and close this.

matmarex closed this task as Resolved.Apr 3 2017, 11:21 PM