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

Upgraded omero-web dependencies #433

Merged
merged 9 commits into from
Feb 9, 2023
Merged

Upgraded omero-web dependencies #433

merged 9 commits into from
Feb 9, 2023

Conversation

kyleBalisWest
Copy link
Collaborator

@kyleBalisWest kyleBalisWest commented Jan 5, 2023

Upgraded omero-web jquery dependencies. See upgrade spreadsheet for a detailed list of changes.

Library Current Version Latest Version Link Notes Updated
jquery-ui 1.12.1 1.13.2 https://jqueryui.com/download/ Removed background and border from .ui-state-default
Removed background and color from .ui-widget-content
Removed border from .ui-state-active
Yes
jquery 3.5.1 3.6.2 https://jquery.com/download/ No changes needed because jQuery.htmlPrefilter isn't used Yes
d3 3.5.17 7.7.0 https://d3js.org/ No longer d3.scale.linear, now is d3.scaleLinear
No longer d3.svg.area, now is d3.area
No longer d3.svg.line, now is d3.line
Yes
farbtastic 1.2 1.2 https://acko.net/blog/farbtastic-jquery-color-picker-plug-in/ Yes
hammer 2.0.2 2.0.8 https://hammerjs.github.io/ Yes
jquery.chosen 1.8.7 1.8.7 https://harvesthq.github.io/chosen/ Yes
jquery.flot 0.8.3 0.8.3 https://www.flotcharts.org/ Weirdness with npm version and github release version
Sticking to github release for now
Yes
jquery.jstree 3.3.10 3.3.12 https://www.jstree.com/ Yes
jquery.tablesorter 2.0.3 2.31.3 https://mottie.github.io/tablesorter/docs/ No
JQuerySpinBtn 1.3a ? No
panojs 2.0.0 3.0.0 https://dimin.net/software/panojs/ Staying at 2.0.0 to allow for irregular tile sizes No
raphael 2.1.0 2.3.0 https://dmitrybaranovskiy.github.io/raphael/ Yes
underscore 1.13.1 1.13.6 https://underscorejs.org/ Yes
aop 1.3 ? No
jquery.blockUI 2.66.0 2.7 http://malsup.com/jquery/block/ No
jquery.form 4.3.0 4.3.0 https://github.com/jquery-form/form No
jquery.hotkeys 0.8 ? No
jquery.mousewheel 3.0.6 3.1.12 Yes
jquery.quicksearch 1.0 ? No
jquery.selectboxes 2.2.6 ? https://github.com/SamWM/jQuery-Plugins/tree/master/selectboxes No
jquery.ui.touch-punch 0.2.3 ? https://github.com/furf/jquery-ui-touch-punch No

@kyleBalisWest kyleBalisWest marked this pull request as draft January 5, 2023 14:07
@@ -1570,7 +1570,7 @@ def report_settings(module):
"3rdparty/JQuerySpinBtn-1.3a/JQuerySpinBtn.css",
"3rdparty/jquery-ui-1.10.4/themes/base/jquery-ui.all.css",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting these were never updated for the previous jquery-ui upgrade but that things kept working. Is it still worth keeping in sync with the current proposed version of the library?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is interesting. I tried fixing it to see if jquery-ui is loaded to the browser differently since it might be compressed with pipeline but I didn't see a difference.

@@ -1585,20 +1585,20 @@ def report_settings(module):
"3rdparty/jquery-ui-1.10.4/js/jquery-ui.1.10.4.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

@will-moore
Copy link
Member

will-moore commented Jan 11, 2023

Mapr uses the jquery-UI auto-complete, which looks a bit different:

Before this PR:
Screenshot 2023-01-10 at 13 33 20

With this PR:
Screenshot 2023-01-10 at 13 33 00

See #180 (comment) and the commit just before that comment

@chris-allan
Copy link
Member

Not important right now while we're iterating but I would suggest that before this is merged we get all detail out of the Google Spreadsheet as Markdown and into the description of this PR. We will need the content in that form for a migration guide and/or changelog anyway.

this.realXTileSize = this.xTileSize;
this.realYTileSize = this.yTileSize;

this.tileSize = (options.tileSize ? options.tileSize : PanoJS.TILE_SIZE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like PanoJs(viewer, options) no-longer supports options.xTileSize and options.yTileSize?
So in our usage of PanoJS we'll need to use options.tileSize instead, which should be fine for when tile size x and y is the same, but I think there are cases when this isn't true. I don't have a feel for how often this occurs or if there's any way to support it (except maybe not upgrading)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, that's a good point. I don't know much about pyramidal data, so I was wondering how often does the xTileSize and yTileSize differ?

Though, if you don't want to upgrade it should be an easy switch back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyleBalisWest Often enough, see example in #433 (comment) please :)

@will-moore
Copy link
Member

I'm afraid jQuery-UI auto-complete is still off (mapr):

Screenshot 2023-01-19 at 10 25 49

It looks like this is due to the previous jQuery-ui having a display: block on the <a>

Screenshot 2023-01-19 at 10 40 56

Whereas there is no such rule in the updated version:
Screenshot 2023-01-19 at 10 43 56

Probably easiest fix is to add display: block to that dusty.css under .ui-menu-item a?

@jburel
Copy link
Member

jburel commented Jan 19, 2023

  • Preview tab (right-hand panel)
    • when moving the right-hand side of the slider knob to the left, the knob always moves back to the full range after release

PW: WFM, can you maybe add details please ?
PW: Aha, got it, the can confirm the comment of #433 (comment)

@will-moore
Copy link
Member

@jburel I think that sounds like expected behaviour if the right slider value is above the max intensity, the slider will update it's upper limit to be the max of the current slider value or the max intensity value.

@will-moore
Copy link
Member

will-moore commented Jan 19, 2023

Re: Pano.js comment above #433 (comment):
As @pwalczysko noticed: this image https://merge-ci.openmicroscopy.org/web/webclient/?show=image-146365 has:

tile_size": {
"width": 2592,
"height": 134
},

and doesn't show up in the Preview panel

PW: Edit: Other examples are https://merge-ci.openmicroscopy.org/web/webclient/?show=image-169215
https://merge-ci.openmicroscopy.org/web/webclient/?show=image-210315
https://merge-ci.openmicroscopy.org/web/webclient/?show=image-250767

Note: opening the image https://merge-ci.openmicroscopy.org/web/webclient/?show=image-140039 in old viewer of OMERO.web, you see as below (the glitch in left-hand side of the image is not present in iviewer)

Screenshot 2023-01-19 at 12 08 36

@jburel
Copy link
Member

jburel commented Jan 19, 2023

@jburel I think that sounds like expected behaviour if the right slider value is above the max intensity, the slider will update it's upper limit to be the max of the current slider value or the max intensity value.

The problem was noticed when using https://merge-ci.openmicroscopy.org/web/webclient/?show=image-236165

PW: The problem is not caused by the tested PR

@will-moore
Copy link
Member

@pwalczysko For me, the old viewer with that image looks a bit different - Maybe you've got some tiles cached? Anyway, it's also due to Pano.js: In this case the tiles are square: "tile_size": { "width": 1024, "height": 1024 },, so it could be fixed by passing Pano.js the correct (new) options, but I think we need to roll back the Pano.js upgrade anyway, to support non-square tiles of other images.

@pwalczysko
Copy link
Member

@pwalczysko For me, the old viewer with that image looks a bit different - Maybe you've got some tiles cached? Anyway, it's also due to Pano.js: In this case the tiles are square: "tile_size": { "width": 1024, "height": 1024 },, so it could be fixed by passing Pano.js the correct (new) options, but I think we need to roll back the Pano.js upgrade anyway, to support non-square tiles of other images.

No, I tried to clear cache and also a new browser, the problem persists. The issue here might be that I have zoomed in a little, otherwise the problem does not manifest.

@pwalczysko
Copy link
Member

pwalczysko commented Jan 23, 2023

There are still some issues with icons on the Preview pane, see for example images such as https://merge-ci.openmicroscopy.org/web/webclient/?show=image-146365

The icons on merge-ci

Screenshot 2023-01-23 at 12 08 47

And errors in the Console

Screenshot 2023-01-23 at 12 11 32

The icons on lates-ci

Screenshot 2023-01-23 at 12 09 47

@@ -552,7 +552,7 @@ jQuery.fn.viewportImage = function(options) {
imageWidth : myPyramid.width,
imageHeight : myPyramid.height,
initialZoom : init_zoom,
staticBaseURL : mediaroot+'3rdparty/panojs-2.0.0/images/',
staticBaseURL : mediaroot+'3rdparty/panojs-2.0.0/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tracked it down to this change that didn't get reverted when you returned to panojs-2.0.0 and that is breaking the icons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah I missed that, thanks for finding it @will-moore. It should be fixed now!

@pwalczysko
Copy link
Member

All looks good to me now, thanks @kyleBalisWest

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good - Just the auto-complete issues (and suggested fix) at #433 (comment) to address

@sbesson
Copy link
Member

sbesson commented Jan 25, 2023

All looks good - Just the auto-complete issues (and suggested fix) at #433 (comment) to address

@will-moore do you know if there anything in vanilla OMERO.web that allows auto-completion and would be affected by this issue? If not, could the display:block not be added to the relevant CSS element in the omero-mapr?

@will-moore
Copy link
Member

@sbesson That would mean that the web upgrade is a breaking change and you wouldn't want to upgrade omero-web without also upgrading mapr. But how to enforce that.
It adds complexity unnecessarily I think.
Just add display: block to .ui-menu-item a in dusty.css should fix it (going on my comment above)

@chris-allan
Copy link
Member

That would mean that the web upgrade is a breaking change and you wouldn't want to upgrade omero-web without also upgrading mapr.

We are upgrading several core libraries in this PR and #435 is also likely to go in to the next version. These are both breaking changes in my opinion. Do you disagree?

Consequently, yes, I would expect you would not want to upgrade omero-web without also upgrading mapr, no? And you would want this new mapr version to depend on the at least the new minor version. This is the same pattern we followed for the Django 3.2 work in #356. Am I missing something here?

@will-moore
Copy link
Member

I guess I don't understand the objection to fixing the issue in this PR?

@sbesson
Copy link
Member

sbesson commented Jan 30, 2023

I guess I don't understand the objection to fixing the issue in this PR?

Having looked a bit the history, I found the root of this issue precedes this PR and turned out to already be a problem during the former jquery upgrade in #180. At the time, patches were introduced directly in the CSS to meet the expectations of the mapr plugin, more specifically in

/* EDIT - copied from jquery-ui-1.10.4/themes/base/jquery.ui.menu.css */
.ui-menu .ui-menu-item a {
text-decoration: none;
display: block;
padding: 2px .4em;
line-height: 1.5;
min-height: 0;
font-weight: normal;
}
and
/* Fixes auto-complete menu in mapr */
.ui-menu-item a.ui-state-active{
background: #D4D6D8;
margin: 0;
}
In other terms, we have been maintaining a vendored fork of jquery-ui. More than the proposed fix itself, my main concern has to do with perpetuating this practice.

Let's assume another Web app developed by the community was relying on the internals of d3 and using d3.scale.linear which has been dropped in the upgraded version. Would we consider introducing some form of compatibility layer in OMERO.web allowing them to continue using this API in the foreseeable future even as we keep upgrading the library? In that case, I think we would all agree the maintenance cost is really high and the plugin should be responsible for upgrading its internals and making another release compatible with the new OMERO.web release. As far as I understand, the situation is no different for mapr which effectively relies on some jquery-ui styling which has been change upstream almost a decade ago.

Beyond this particular fix, paraphrasing @chris-allan, I also think we want to be transparent about the fact there are breaking changes associated with the library upgrades proposed here. I would be very careful about conveying the misleading idea that consumers/app developers can just upgrade OMERO.web without thinking about the nature of these changes and the implications on their own plugins. In many ways, having a handful of OMERO.web apps in our own ecosystem that also need to be updated and released is a healthy process as it forces us to think how to communicate the upgrade process to our community.

@will-moore
Copy link
Member

OK, I guess this PR is good to merge then

@knabar
Copy link
Member

knabar commented Jan 31, 2023

Thanks everyone for the PR reviews!

@knabar knabar dismissed will-moore’s stale review January 31, 2023 09:52

Not adding custom plugin CSS at this time

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

Successfully merging this pull request may close these issues.

7 participants