Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

menubar buttons do not react on click on chrome 55 (solved with PR #9219 in master) #9182

Closed
level420 opened this issue Sep 20, 2016 · 27 comments

Comments

Projects
None yet
7 participants
@level420
Copy link
Member

commented Sep 20, 2016

Verified on windows 7x64 with chrome 54.0.2840.27 beta-m (64-bit)

To reproduce open http://www.qooxdoo.org/devel/demobrowser/#widget~MenuBar.html or http://www.qooxdoo.org/current/demobrowser/#widget~MenuBar.html and click on e.g. the "File" menu entry.

The file "File" menu button gets hovered, but does not open the menu pane with the sub menues.

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

I may have come in because of a deprecation warning which now shows up in the console:

Event.js:52 Navigator.pointerEnabled is a non-standard API added for experiments only. It will be removed in near future
.qx.bom.client.Event.getMsPointer() @ Event.js:52qx.core.Environment.get() 
@ Environment.js:906defer 
@ Environment.js:83(anonymous function) 
@ Bootstrap.js:205qx.Bootstrap.addPendingDefer() 
@ Bootstrap.js:431qx.Bootstrap.define() 
@ Bootstrap.js:204(anonymous function) 
@ Environment.js:39
Native.js:58 003612 qx.core.Init: Load runtime: 3611ms
@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

Works as expected without showing the bug on windows 7x64 with chrome Version 53.0.2785.116 m (64-bit)

@cajus

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

Not reproducible on linux with latest Chrome 54 beta.

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

Reproducible on windows 10 with chrome 54 beta.

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

It seems that in qx.ui.form.MenuButton at https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/form/MenuButton.js#L190 the _onPointerDown event handler is processed twice, which means first opening and then closing the menu.

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

let's see if this persists in the next beta.

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2016

the issue is still there on chrome 54.0.2840.34 beta-m (64-bit) on windows 7 x64

@level420

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2016

not reproducible on OS X 10.11.6 with chrome Version 54.0.2840.34 beta (64-bit)

@level420 level420 added the OS:Windows label Sep 22, 2016

@level420 level420 changed the title menubar buttons do not react on click on chrome 54 beta menubar buttons do not react on click on chrome 54 beta on ms windows Sep 26, 2016

@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

the issue is still there on chrome Version 54.0.2840.50 beta-m (64-bit) on windows 7 x64

@slachtar

This comment has been minimized.

Copy link

commented Oct 17, 2016

the workaround works great. thanks.

@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

@slachtar could you please tell me which browser version you are using on which OS exactly?

I'm asking because I cant't reproduce the issue anymore on windows 7x64 with chrome version 54.0.2840.59 m (64-bit) which is the stable release, but can still reproduce it with version 54.0.2840.59 beta-m (64-bit).

@level420 level420 changed the title menubar buttons do not react on click on chrome 54 beta on ms windows menubar buttons do not react on click on chrome 54 beta and chrome 55 beta on ms windows Oct 21, 2016

@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

The issue is again there with chrome 55.0.2883.21 beta-m (64-bit) on windwos 7x64.

@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

I think I found the change in chrome which causes this issue:
https://developers.google.com/web/updates/2016/10/pointer-events

It may be that at some point they already shipped the change with chrome 54 beta and decided to retract it for 54 stable.

We can detect the new behaviour in chrome by checking window.PointerEvent as documented here:
https://developers.google.com/web/updates/2016/10/pointer-events#browser_support.

The solution would be to add this check to qx.bom.client.Event.getMsPointer to return true which leads to qx.core.Environment.get("event.mspointer") for chrome 55 on.

I'll create a PR for this.

@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2016

As opera also uses blink, this will hit opera on version 42. See http://caniuse.com/#feat=pointer

cajus added a commit that referenced this issue Oct 24, 2016

Merge pull request #9219 from level420/level420-chrome-55-unified-poi…
…nter-input

Issue #9182 Detect unified pointer input model for Chrome >= 55
@level420

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2016

Fixed via #9219

@level420 level420 closed this Oct 25, 2016

@level420

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

One additional note:

Chrome 55 was released today and it seems that they decided to to retract the pointer event feature in the stable version 55.0.2883.75, while it is still there in the also today released 55.0.2883.75 beta from the beta channel.

This shows that in most cases it is better to use feature detection and not rely on browser version detection.

But we are prepared now, at least for the upcoming pointer events in chrome.

@vvandens

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2016

Wow! you saved my day. Thanks!

@level420

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2016

@vvandens you're welcome!

@vvandens

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2016

@level420 a side question : I try to cleanly patch an application that must stay on 5.0.1 release until next release is available. I simply copy-pasted the latest static method getMsPointer you fixed in one of the application class, then when the application starts, I call :

qx.core.Environment.invalidateCacheKey("event.mspointer");
qx.core.Environment.add("event.mspointer", my.own.Class.getMsPointer);

However, the bug is still there in the application.
Tested the devel demo browser and everything is fine there, so I suspect I must be missing something... Any hint ?
[edit] I think It's too late since all bootstrap initializations have been performed when it comes to my application class...
[solved] For the record, here is the solution that works for me. I define a "patch" class in my application :

qx.Class.define("org.jspresso.framework.patch.MEvent", {
  statics: {
    /**
     * Checks if MSPointer events are available.
     *
     * @internal
     * @return {Boolean} <code>true</code> if pointer events are supported.
     */
    getPatchedMsPointer: function () {
      // Fixes issue #9182: new unified pointer input model since Chrome 55
      // see https://github.com/qooxdoo/qooxdoo/issues/9182
      if ("PointerEvent" in window) {
        return true;
      }
      if ("pointerEnabled" in window.navigator) {
        return window.navigator.pointerEnabled;
      } else if ("msPointerEnabled" in window.navigator) {
        return window.navigator.msPointerEnabled;
      }
      return false;
    }
  },

  defer: function (statics) {
    qx.core.Environment.add("event.mspointer", statics.getPatchedMsPointer);
  }
});

Then in my application class :

/*
* @require(org.jspresso.framework.patch.MEvent)
*/

This will fill-in the Environment before the qx classes.

@alp82

This comment has been minimized.

Copy link

commented Dec 7, 2016

Thx @vvandens for your workaround solution. I tried to use this method but menues are still not working. While debugging i saw that qx.core.Environment.add did nothing because the _checks array already contained the event.mspointer value.

I replaced the defer method with:

defer: function (statics) {
    qx.core.Environment.invalidateCacheKey("event.mspointer");
    delete qx.core.Environment.getChecks()['event.mspointer'];
    qx.core.Environment.add("event.mspointer", statics.getPatchedMsPointer);
}

But still no dice, menues don't work. Any idea?

@level420

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2016

@alp82 it seems that @vvandens solution managed to add the key function before the original code is setting the key.

@vvandens could you help here? thank you!

@level420 level420 changed the title menubar buttons do not react on click on chrome 54 beta and chrome 55 beta on ms windows menubar buttons do not react on click on chrome 55 Dec 7, 2016

@level420 level420 reopened this Dec 7, 2016

@level420

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2016

reopened due to discussion

@eflowbeach

This comment has been minimized.

Copy link

commented Dec 11, 2016

Not a permanent fix, but this override seemed to temporarily patch my 5.0.1 app:

qx.Class.define("edd.JQx.MenuButton",
{
  extend : qx.ui.form.MenuButton,
  construct : function(label, icon, menu)
  {
    var me = this;
    this.base(arguments, label, icon);

    // Initialize properties
    if (menu != null) {
      this.setMenu(menu);
    }
    me.addListener("pointerdown", function(e) {
      me.open();
    }, this);
  }
});

This probably creates more problems than it fixes though.

miracle2k added a commit to aspectit/qooxdoo that referenced this issue Dec 11, 2016

@level420

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2016

@eflowbeach you could add the following lines https://github.com/qooxdoo/qooxdoo/pull/9219/files to your local copy of qooxdoo.

@eflowbeach

This comment has been minimized.

Copy link

commented Dec 13, 2016

@level420 After I posted my comment, I realized that it was only a few lines of code. Thanks for the suggestion, it works like a charm now!

@level420 level420 removed the OS:Windows label Dec 13, 2016

@vogelor

This comment has been minimized.

Copy link

commented Dec 15, 2016

i am using Version 55.0.2883.87 m at windows 10 64 bit and the menu-button stopped working. After patching #9219 by hand, it works again! so thank you for your great job!

@level420 level420 changed the title menubar buttons do not react on click on chrome 55 menubar buttons do not react on click on chrome 55 (solved with PR #9219 in master) Dec 16, 2016

adamansky added a commit to Softmotions/qooxdoo that referenced this issue Dec 21, 2016

cajus added a commit that referenced this issue Jan 10, 2017

@level420

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

@level420 level420 closed this Jan 27, 2017

vegansk added a commit to eldis/qooxdoo that referenced this issue Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.