Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#9241 closed bug (fixed)

Dialog: UI dialog inheritance causes undefined property '_focusTabbable' in IE9

Reported by: creage Owned by:
Priority: minor Milestone: 1.11.0
Component: ui.dialog Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:

Description

  1. Create a new widget inherited from UIDialog.
  2. Init two instances of that widget, and try to open them from code.
  3. Close first one from code.

Actual: IE9 trows an error of undefined property _focusTabbable. Expected: Inherited dialogs work without errors.

See http://jsfiddle.net/NDFCd/4/

Tested on IE9 having Compatibility Mode disabled.

Change History (16)

comment:1 Changed 4 years ago by creage

Seems like it only fails when you do these steps:

  1. Create a new widget inherited from UIDialog.
  2. Init an instance of that widget, open it.
  3. Init an instance of standard UIDialog, open it
  4. Now close the inherited instance, created on step #2

comment:2 Changed 4 years ago by scottgonzalez

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #9097.

comment:3 Changed 4 years ago by creage

It is not a duplicate, since #9097 is fixed, but this bug is still reproducible.

comment:4 follow-up: Changed 4 years ago by scottgonzalez

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I don't know why this would be browser-specific.

comment:5 Changed 4 years ago by scottgonzalez

  • Status changed from reopened to open

comment:6 in reply to: ↑ 4 Changed 4 years ago by creage

It is reproducible on IE10 as well - seems like MS-style issue.

comment:7 Changed 3 years ago by jzaefferer

This is the line throwing the error: https://github.com/jquery/jquery-ui/blob/1.10.3/ui/jquery.ui.dialog.js#L722 via a jsbin that includes a non-minified jQuery UI version: http://jsbin.com/iziraje/1

Considering the context, its not surprising that this only fails in IE (I've tested IE8). In Chrome that line isn't executed when loading the test page.

The problem is the call to .data( widgetFullName ). An inherited dialog instance stores its data under a different key, here "sj-blocker", so that access fails.

This kind of namespacing problem isn't exaclty new. For grid dataviews, we ended up forcing consistent event prefixes, to make events reliable even in the context of inheritance, e.g. https://github.com/jquery/jquery-ui/blob/0d60241a62bf4511708b2ab2a1a840296da31d5c/grid-spf/slideshow-dataview.js#L14 Maybe we need something comparable here to get a little closer to actual polymorphism?

comment:8 Changed 3 years ago by creage

Personally I'm fixing this issue by rewriting _allowInteraction method

_allowInteraction: function (event) {
			
	var result = this._super(event);
			
	if (!result) { // replicate ui-dialog instance to fix jQueryUI
				
		var tabbableWidgetInstance = $('.ui-dialog:visible:last .ui-dialog-content'),
			inst = tabbableWidgetInstance.data(this.widgetFullName);
				
		if (!inst) {
			tabbableWidgetInstance.data(this.widgetFullName, tabbableWidgetInstance.data('ui-dialog'));
		}
	}

	return result;
}

comment:9 Changed 3 years ago by jzaefferer

I've been thinking about this some more. One possibility is to also store inherited widgets under the widgetFullName of their parent. So all instances of dialog, inherited or not, would be stored as ui-dialog. That would allow dialog itself to access other dialog instances with .data( "ui-dialog" ). Storing both would still allow access to the actual name. That would fix this particular issue and would improve polymorphism in general, since you could do this:

$.widget( "sj.customDialog", $.ui.dialog, {
  options: {
    autoOpen: false
  }
}):
var div =$("div").customDialog();
div.dialog("open");

On the implementation side:

  • in $.widget, if base is not $.Widget, find the base that inherits from $.Widget (in case of deeper inheritance chains) and store its widgetFullName as baseFullName on the constructor.prototype
  • in $.widget.bridge, if object.prototype.baseFullName exists, call $.data( this, object.prototype.baseFullName, instance ), after the other $.data call, storing the same instance (reuse existing instance variable)
  • in dialog, use .data( "ui-dialog" ) instead of .data( this.widgetFullName )

Note that there's no other change needed to support method calls on the parent type. Pretty nice, unless I'm missing something.

Also we should extend the jQuery.widget documentation or learn articles with details on polymorphism through the widget bridge.

comment:10 Changed 3 years ago by scottgonzalez

That's not a valid approach, since it won't work with things like elem.draggable().resizable() where both draggable and resizable have a common base.

comment:11 Changed 3 years ago by jzaefferer

Valid point. Any ideas what we could do instead?

comment:12 Changed 3 years ago by scottgonzalez

We can track the dialog instance that was most recently interacted with, and just call _focusTabbable() on that instance. If we go with that, we should wait for https://github.com/jquery/jquery-ui/pull/1108 to land first, since we can leverage _trackFocus().

comment:13 Changed 3 years ago by jzaefferer

Discussed this with Scott: In _trackFocus() we store the active instance via this.document.data("ui-dialog-recent", ...);. That data attribute is an array. If the instance already exists in there, we remove it. Then we add it at the end.

In the overlay/focusin handler, we call _focusTabbable on the last element in the array. There must be at least one element in the array.

We also remove the instance from that array when when closing a dialog.

comment:14 Changed 3 years ago by jzaefferer

  • Summary changed from UI dialog inheritance causes undefined property '_focusTabbable' in IE9 to Dialog: UI dialog inheritance causes undefined property '_focusTabbable' in IE9

comment:15 Changed 3 years ago by Jörn Zaefferer

  • Resolution set to fixed
  • Status changed from open to closed

Dialog: Keep track of instances to focus when elements outside the dialog get focus. Works with inheritance. Adds tests for both. Fixes #9241 - Dialog: UI dialog inheritance causes undefined property '_focusTabbable' in IE9

Changeset: 1096f19f37d6075328509d62a4c2c6d6a53d4b37

comment:16 Changed 3 years ago by scottgonzalez

  • Milestone changed from none to 1.11.0
Note: See TracTickets for help on using tickets.