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

dijit popup menus fail on current Chrome Beta (107) #201

Open
schallm opened this issue Oct 20, 2022 · 11 comments
Open

dijit popup menus fail on current Chrome Beta (107) #201

schallm opened this issue Oct 20, 2022 · 11 comments

Comments

@schallm
Copy link
Contributor

schallm commented Oct 20, 2022

Menus with popup children fail to show correctly, and warnings are seen in the console output on some installs of Chrome Beta channel (currently 107).

image

Download Chrome Beta channel (currently 107) for Windows (maybe other OSs?) and load the following tutorial:

https://dojotoolkit.org/documentation/tutorials/1.10/menus/demo/nestedProgMenu.html

Some machines work fine, however some break as described above. I assume it is Chrome testing on a certain percent of machines? I believe you will only see the issue if the following returns true in the Chrome Beta console window:

 Element.prototype.hasOwnProperty("popUp");

I believe it is due to this feature currently being tested: https://chromestatus.com/feature/5463833265045504

Is this a bug Chrome will fix or does dijit need to use a different attribute?

@dylans
Copy link
Member

dylans commented Oct 21, 2022

We'll likely need to rename this, unless the Chrome team determines that this breaks enough existing websites such that they suggest renaming the feature.

@schallm
Copy link
Contributor Author

schallm commented Oct 26, 2022

Chrome stable 107 does not show the issue. How do we move forward? No way of know if they will rename the feature, correct? Does dijit just need to change? Looks like they are planning to ship with version 110 (~3 months away based on every four weeks for major releases.).

What would be the suggested new marker attribute? Maybe dijit-popup or dojo-popup? Have there been similar breakages in the past?

@wkeese
Copy link
Member

wkeese commented Nov 6, 2022

I can't reproduce it, but Element.prototype.hasOwnProperty("popUp") returns false for me on Chrome and even Chrome Canary.

The thing is though that I don't see any attribute named popup and I don't see any code that sets such an attribute either.

@schallm
Copy link
Contributor Author

schallm commented Nov 7, 2022

I have had some co-workers try. So far we have a 3 machines testing. Only 1 Chrome Beta (currently 108) is showing the issue. Here is a screenshot of the issue from Windows running Version 108.0.5359.29 (Official Build) beta (64-bit):

chrome-108

@wkeese
Copy link
Member

wkeese commented Nov 8, 2022

OK, but where is this popup attribute/property getting set? I don't see it anywhere in the code.

It does sound vaguely familiar though, I wonder if I removed it but you still have it because you're using an old version of the code.

@schallm
Copy link
Contributor Author

schallm commented Nov 8, 2022

Looks like when creating a new PopupMenuItem and passing an object with popup property,

Example shown here in dijit's test: https://github.com/dojo/dijit/blob/master/tests/test_Menu.html#L78

When widget is being constructed, the WidgetBase.create function calls _applyAttributes:

_applyAttributes: function(){

The loop at the bottom of the function loops over params and creates attributes.

dijit/_WidgetBase.js

Lines 504 to 506 in 8ab4cdc

for(key in params){
this.set(key, params[key]);
}

"popup" is one of the keys in params.

popup-params-image

@wkeese
Copy link
Member

wkeese commented Nov 8, 2022

Hmm, OK, but I actually was looking for the code that calls that code, somewhere where "popup" is actually written.

@mpajer-gti
Copy link

The problem is here:
https://github.com/dojo/dijit/blob/master/_WidgetBase.js#L862

names.l in attrsForTag is true
so next if(map != null){ is also true and this._attrToDom(name, value, map); is invoked
Which causes Found a 'popup' attribute with an invalid value error

@wkeese
Copy link
Member

wkeese commented Nov 11, 2022

Well, but I actually was looking for the code that calls that code, somewhere where "popup" is actually written.

@mpajer-gti
Copy link

Example code was mentioned in the first comment:
https://dojotoolkit.org/documentation/tutorials/1.10/menus/demo/nestedProgMenu.html

The issue is when the "popup" is used in constructor props
w = new PopupMenuItem({ id: "task", label: "Task", popup: taskMenu })
or as setter
w.set("popup", taskMenu);

@wkeese
Copy link
Member

wkeese commented Nov 11, 2022

Ah sorry, I see now. So that's hard to change (without breaking backwards compatibility) because it's part of the API.

Maybe would need to deprecate it (but keep supporting it) and create a new property that does the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants