Opened 6 years ago

Closed 6 years ago

#17599 closed defect (fixed)

native focusin support incorrectly false in ie11

Reported by: Colin Snover Owned by: Colin Snover
Priority: blocker Milestone: 1.8.6
Component: Events Version: 1.9.1
Keywords: Cc: Clement Mathieu
Blocked By: Blocking:

Description

This is due to a bad inference that document.attachEvent means IE. It does not. Don’t do this.

Attachments (1)

event-focusin.html (1.0 KB) - added by haysmark 6 years ago.
Partial test case. First test fails in Chrome due to lack of domReady.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by Colin Snover <github.com@…>

Resolution: fixed
Status: newclosed

In e245ce5c80b85351642fd1ffc18a2d77bbbb0770/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:2 Changed 6 years ago by Colin Snover <github.com@…>

In ca391f932e3a3f032dd8b84566c4176857eb8ac3/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:3 Changed 6 years ago by Colin Snover <github.com@…>

In 2536e5cd0935cc7356d584bab6ac2c73d3ad6164/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:4 Changed 6 years ago by Colin Snover <github.com@…>

In f774568707ea95b9a56bf646b2ce46e1c57907f8/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:5 Changed 6 years ago by mahays0 <mahays0@…>

In 90ee9bf716b19e716841fac5355cb979f2a94f2d/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:6 Changed 6 years ago by mahays0 <mahays0@…>

In c40eec64afdab97a34d22f5a7f28605e4028e9d9/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:7 Changed 6 years ago by bill

Milestone: 1.9.21.8.6

comment:8 Changed 6 years ago by cjolif

This patch I think introduces an issue when dojo/on is loaded but the DOM is not ready yet. Indeed the module does doc.body.appendChild() but do not require domReady plugin.

comment:9 Changed 6 years ago by cjolif

Resolution: fixed
Status: closedreopened

comment:10 Changed 6 years ago by bill

This line of code executes as soon as on.js loads (even if nobody calls on() or on.emit(), etc.):

var captures = has("event-focusin") ? {} : {focusin: "focus", focusout: "blur"}

That has() call executes the event-focusin callback, which references doc.body, even though body might not be defined yet.

It's hard to make an isolated test case for a race condition like this but we've seen the failure in one of our products.

Changed 6 years ago by haysmark

Attachment: event-focusin.html added

Partial test case. First test fails in Chrome due to lack of domReady.

comment:11 Changed 6 years ago by haysmark

I attached a test case to reproduce the issue reliably in Chrome. IE doesn't see the issue because the onfocusin check short circuits the test. I suppose we could just add domReady! to on.js as a quick fix.

comment:12 Changed 6 years ago by Colin Snover <github.com@…>

In 31aab4cc1bb93592d1cb6a3c4e34f612f36d525a/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:13 Changed 6 years ago by Colin Snover <github.com@…>

In 08187b88273f09326e5ebc8ea03eecd592e7eaca/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:14 Changed 6 years ago by Colin Snover <github.com@…>

In 5a93bd37cbda573e5c3a6be895336dc42eb46bca/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:15 Changed 6 years ago by Colin Snover

Resolution: fixed
Status: reopenedclosed

That should be it.

comment:16 Changed 6 years ago by bill

Hmm, won't that break the (legacy) sync loader? Something about a deadlock waiting for dojo/domReady to fire, or maybe the opposite, where the module factory executes even though the DOM's not ready. That's the reason that we aren't using dojo/domReady! in the define() lists for any of the other modules.

Version 0, edited 6 years ago by bill (next)

comment:17 Changed 6 years ago by bill

Resolution: fixed
Status: closedreopened

See dijit/tests/form/test_FilteringSelect.html as an example of something broken by your change. It no longer loads, at least on my machine. It gets stuck referencing dijit.form.FilteringSelect? where dijit.form is undefined.

comment:18 Changed 6 years ago by bill

Presumably another problem with your feature test is that it will break a page that initially sets focus. For example, a page with:

<input id="autofocusInput" type="text" autofocus="autofocus">

Or possibly a page with some code like this that runs before your feature test:

<body>
   ...
   <script>window.myNode.focus();</script>
</body>

From all your checkins, it seems you really want to make Webkit use focusin/focusout events, even though AFAIK Webkit was working fine to begin with. But, a more practical approach would be to just make the test be:

  • "onfocusin" in element - makes IE11 using native focusin/focusout, addressing the complaint of this ticket
  • or "onfocusin" in element || has("webkit") - also makes webkit use native focusin/focusout
  • !has("mozilla")
  • leave the code as it was originally. is there actually a bug here, or are you just trying to make things more efficient?

comment:19 in reply to:  18 Changed 6 years ago by Colin Snover

Replying to bill:

Presumably another problem with your feature test is that it will break a page that initially sets focus. For example, […]

I designed this code to not break pages in the way you are describing: The focused element is stored before changing focus and restored afterward.

What the code might do is cause an additional set of focus/blur events that the user was not expecting. If someone only uses dojo/on then it is not a big deal; it might be a bigger deal in other cases.

BTW, modifying focus to detect features is not without precedent, there is at least one feature detect in Modernizr (oninput test) that does it.

From all your checkins, it seems you really want to make Webkit use focusin/focusout events, even though AFAIK Webkit was working fine to begin with.

Yes, the goal was to take a feature test that was wrong, and fix it to be right, since the feature exists in these browsers and we should be able to detect and use it correctly.

But, a more practical approach would be to just make the test be: "onfocusin" in element - makes IE11 using native focusin/focusout, addressing the complaint of this ticket

This will probably be what happens, since everything else is too wrapped in spaghetti to allow the feature test to work.

  • or "onfocusin" in element || has("webkit") - also makes webkit use native focusin/focusout
  • !has("mozilla")

UA sniffs for features are NEVER acceptable in a library. The only time it becomes OK to do this is if you have an untestable defect (i.e. memory leak).

  • leave the code as it was originally. is there actually a bug here, or are you just trying to make things more efficient?

The focus behaviour was not the same using capture phase, which broke tests, which is how I identified this defect originally. In any case the old code was obviously wrong so it should not stay that way.

comment:20 Changed 6 years ago by cjolif

Cc: Clement Mathieu added

comment:21 Changed 6 years ago by cjolif

So what are we doing? We have here a regression on a release and we have IE11 support to 1.8 that is blocked by that. So it sounds to me we should accelerate the resolution here?

But, a more practical approach would be to just make the test be: "onfocusin" in element - makes IE11 using native focusin/focusout, addressing the complaint of this ticket

This will probably be what happens, since everything else is too wrapped in spaghetti to allow the feature test to work.

If we have no better idea for now, why not doing that and just committing it? We can always improve things if we have a brighter idea later but at least we would not be blocked anymore?

comment:22 Changed 6 years ago by Colin Snover <github.com@…>

In deb09c12e66bc1c0716f910dd0169de6dfc19dfc/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:23 Changed 6 years ago by Colin Snover <github.com@…>

In df47713a809bb5dfdd9a6dad472b6693c501b866/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:24 Changed 6 years ago by Colin Snover <github.com@…>

In 81e1c7d6b52e2f42a3bbc8259f875bd83aaa8541/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:25 in reply to:  21 Changed 6 years ago by Colin Snover

Resolution: fixed
Status: reopenedclosed

Replying to cjolif:

So what are we doing? We have here a regression on a release and we have IE11 support to 1.8 that is blocked by that. So it sounds to me we should accelerate the resolution here?

But, a more practical approach would be to just make the test be: "onfocusin" in element - makes IE11 using native focusin/focusout, addressing the complaint of this ticket

This will probably be what happens, since everything else is too wrapped in spaghetti to allow the feature test to work.

If we have no better idea for now, why not doing that and just committing it? We can always improve things if we have a brighter idea later but at least we would not be blocked anymore?

You’re right. I have made the commit to just use onfocusin for now. Sorry for the delay.

Note: See TracTickets for help on using tickets.