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

Javascript libraries update #677

Merged
merged 20 commits into from Apr 9, 2021
Merged

Conversation

twhitehead
Copy link
Contributor

Update to newer versions of the various javascript libraries used.

I undertook this as our security people flagged our SHARCNET/Compute Canada hydra server as a XSS security risk due to the use of jquery 1.x. I'm not certain I agree XSS concerns apply to hydra, but figure the update was worthwhile in its own right as the libraries had fallen multiple major versions behind.

You can check out our hydra server to see a running version of these patches on top of the hydra server packaged in 19.03. I rebased that patch set onto master for this pull request. I haven't actually tried it, but the rebase was 100% clean except for a missing c.uri_for call that had been added.

@edolstra
Copy link
Member

A few comments from looking at https://hydra.sharcnet.ca:

  • The pagination buttons at the bottom of https://hydra.sharcnet.ca/steps are rendered as lists.

  • The header in the "Reproduce this build" popup looks wrong.

@twhitehead
Copy link
Contributor Author

Thanks. Fixed now. I also created you a temporary account on the server so you can fully dig through all the pages, dialogues, and such. Let me know if you find anything else.

@twhitehead
Copy link
Contributor Author

@edolstra expect you have been just busy with things, but it's been 4 months now, so just wondering what the thoughts are on this?

We've been running it on our server and haven't noticed any issues yet. Noticed a minor conflict had arisen against master, so just finished resolving that.

Thanks!

@twhitehead
Copy link
Contributor Author

Actually, I just finished breaking our hydra server by pushing this branch to it to double check my conflict resolution.

Looks like it is just a newer perl dependency (Net/Prometheus.pm) that is missing from my override preventing startup.

I probably won't get it resolved to tomorrow though.

@twhitehead
Copy link
Contributor Author

Updated the package dependencies for the newer version and now our hydra server is back up for your perusal. Thanks.

@twhitehead
Copy link
Contributor Author

@edolstra any chance this could get merged if I fixed the latest set of conflicts that have arisen in the since I lasted fixed conflicts back in January?

It looks like there was a branch created back in 2013 to attempt to update to boostrap 3, so I was thinking it might have been something you were wanting.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/calling-all-hydra-users/11851/5

src/root/Makefile.am Outdated Show resolved Hide resolved
@grahamc
Copy link
Member

grahamc commented Mar 18, 2021

I gave rebasing this a shot, and published my rebase here: https://github.com/grahamc/hydra/pull/new/twhitehead-javascript-updates. I took a look at this with @samueldr and we found a number of issues. Most of them are minor, but one is quite major.

Most significantly: clicking tabs don't update the URL.

For example, visit https://hydra.nixos.org/build/137117854 and select Details/Inputs/etc. It'll update the URL, and you can deep-link to a page. Bootstrap 4 seems to eliminate that behavior. Ideally there is a way to bring it back without too much work, but I found https://webdesign.tutsplus.com/tutorials/how-to-add-deep-linking-to-the-bootstrap-4-tabs-component--cms-31180 and it is pretty nasty. I think this problem is significant enough to block this PR.

Important if the deep-linking is fixable

Entering a nix-shell and running the getting started steps should make the web UI work correctly. ie:

$ nix-shell
$ ./bootstrap
$ configurePhase # NOTE: not ./configure
$ make
$ foreman start

Less important things that can be easily fixed

These can probably be each fixed by search and replace.

Button styling

Some button widgets are missing the buttoney display treatment. Example from a build page:

image

Error / warning label styling

Some labels like warnings and errors are no longer looking like labels. Example from a project page:

image

Project page's default tab state isn't correct

On a project page like /projects/nixos, the default loaded tab isn't "selected":

image

Bugs that are already bugs that I don't expect this PR to fix

...but are maybe a bit more pronounced with the new styles.

Zebra stripes on the project list and jobset list pages are mis-striped due to hidden jobsets #578

image

vs:

image

More button & Jobs header on a jobset don't extend far enough

image


Overall, I think if we can fix the deep linking problem we could merge this. The new style is definitely roomier than the old style, but @samueldr pointed out that it is also much more touch-friendly. It took a few minutes to adjust to the new size, but I don't mind it and actually kind of like it by now.

@twhitehead
Copy link
Contributor Author

The w3 site has a bit of javascript to restore deep linking functionality.

var url = window.location.href;
if (url.indexOf("#") > 0){
     var activeTab = url.substring(url.indexOf("#") + 1);
     $('.nav[role="tablist"] a[href="#'+activeTab+'"]').tab('show');
}

$('a[role="tab"]').on("click", function() {
     var newUrl;
     const hash = $(this).attr("href");
     newUrl = url.split("#")[0] + hash;
     history.replaceState(null, null, newUrl);
});

I'm not terribly familiar with javascript, but my reading would be

  1. the first bit handles the initial page load by checks the URL for # and then manually activates the associated tab if found, and
  2. the second bit handles the update by updating the URL everytime something in role tab is clicked.

@samueldr
Copy link
Member

samueldr commented Mar 18, 2021

This is not "the w3 site"... Sorry for the nit pick. It's just an arbitrary website full of stolen snippets and tutorials.

It comes, word for word AFAICT from this bootstrap issue:

This is not a good thing. This is re-implementing what was purposefully broken by bootstrap 4. The links already points to anchors. This may work in most browsers, but I wouldn't put any trust in patching on top of the breakage bootstrap 4 did.

I wouldn't feel confident in approving something that broke a fundamental behaviour of the web, even with a patch on top to try and reinstate it.

Furthermore replaceState is the wrong choice, as it replaces the current state instead of pushing on top of the navigation stack, like the native behaviour. (Though I'm observing tabs with bootstrap 2 seems to not push state, so it seems the implementation is the same...)

@twhitehead
Copy link
Contributor Author

twhitehead commented Mar 18, 2021

@samueldr Thanks for the details and sorry for the bad link. I have to confess I am really out of my depth when it comes to this sort of thing. From your comments and the twbs/bootstrap#25220 issue, it looks like this would be more correct/cleaner

$(`a[href="${window.location.hash}"]`).tab('show')

$('a[role="tab"]').on("click", function() {
     history.pushState(null, null, url.split("#")[0] + $(this).attr("href"));
});

With regard to the fundamental behaviour of the the web, I'm not sure I understand what you are saying. I am guessing maybe you are saying the basic idea of using anchors to deep link tabs is breaking a fundamental behaviour of the web, and that is why it was deliberately removed in bootstrap 4. Is this it, and, if so, are you then saying we shouldn't have deep linking period?

@samueldr
Copy link
Member

samueldr commented Mar 18, 2021

Don't worry, the web is way way way more complex than it looks like. So being out of one's depth is not necessarily an issue :).


With regard to the fundamental behaviour of the the web, I'm not sure I understand what you are saying. I am guessing maybe you are saying the basic idea of using anchors to deep link tabs is breaking a fundamental behaviour of the web, and that is why it was deliberately removed in bootstrap 4. Is this it, and, if so, are you then saying we shouldn't have deep linking period?

What I meant to say is that clicking on an anchor link should push the navigation to that anchor in the browser history, and scroll to the anchor. I guess scrolling to the anchor is undesirable with tabs navigation.

Though looking at how it works, now, I see that @edolstra basically implemented it. It looks like bootstrap 2 would, too, have broken the navigation the same way.

So in the end I guess it's not an issue if we fix it in post? uh.

(For the record, I'm not a fan of bootstrap, but I'm still surprised when bootstrap drops the ball...)

And yes, on their example page for bootstrap 2 you can see it's broken. So uh... Looks like we want this to be fixed, which is the initial assumption that holds, but it's fine to do it "in post", which is my assumption that was flawed.

@grahamc
Copy link
Member

grahamc commented Mar 18, 2021

This option looks great, and resolves my largest concern:

$(`a[href="${window.location.hash}"]`).tab('show')

$('a[role="tab"]').on("click", function() {
     history.pushState(null, null, url.split("#")[0] + $(this).attr("href"));
});

@twhitehead
Copy link
Contributor Author

Thanks guys. Interesting that the original behaviour appears to have been JavaScript too. In some sense then it was a bit I missed in my update. I'll try the rebase plus the javaScript on our server next week to see how it goes.

@twhitehead
Copy link
Contributor Author

Okay, I think I've got the deep link stuff working. It is live on our hydra server, so give it a try and let me know.

I'll force push an update. I went through the rebase and made some other tweaks on top of it too. So I'll add some comments about that.

If it all looks good, I'll squash the fixups and push it again.

<div class="btn-group btn-group-toggle col-sm-9" data-toggle="buttons">
<label id="type-flake" class="btn btn-secondary[% IF jobset.type == 1 %] active[% END %]">
<input type="radio" name="type" value="1" [% IF jobset.type == 1 %]checked[% END %]>Flake</button>
<input type="radio" id="editjobsettype" name="type" value="1" [% IF jobset.type == 1 %]checked[% END %]>Flake</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was missing id to tie back to label.

@@ -6,7 +6,7 @@
<div class="form-group row">
<label class="col-sm-3" for="editprojectenabled">Enabled</label>
<div class="col-sm-9">
<input type="checkbox" id="editprojectenabled" name="enabled" [% IF project.enabled %] checked="checked" [% END %]/>
<input type="checkbox" id="editprojectenabled" name="enabled" [% IF create || project.enabled %] checked="checked" [% END %]/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original had create, so added it back. Let me know if this was removed on purpose.

@twhitehead
Copy link
Contributor Author

With respect to the <pre> stuff, I would be tempted to not fight the system and just use the cards and their closest styling.

The .bg-light background option looks pretty close for the light grey background, and we could just switch the evaluation error from the current dark red on light red to the standard white on red danger one.

That is just me though. I'm perfectly happy with whatever you guys want.

@twhitehead
Copy link
Contributor Author

You can remove that rule entirely as the navbar isn't sticky anymore.

Should I strip out as much of that commit as possible, or just the :target?

@samueldr
Copy link
Member

samueldr commented Apr 7, 2021

Only the :target rules. The rest of the commit looks like it hitched a ride and solve other issues, mainly.

@grahamc
Copy link
Member

grahamc commented Apr 7, 2021

I'm excited for this PR, I think once this last bit is sorted we can go ahead and merge.

@samueldr
Copy link
Member

samueldr commented Apr 7, 2021

absolutely no one said:

But why is the navbar not stickied while it was before?

Well, it could be. It's not a good idea. See bugs websites have to work around. Keeping it at the top rather than stickied makes a class of bugs and issues entirely vanish. Thanks for coming to my ted talk.

@grahamc
Copy link
Member

grahamc commented Apr 7, 2021

I didn't even notice the difference. =)

@twhitehead
Copy link
Contributor Author

Okay. I removed the :target rule and took a stab at wrapping all the <pre> tags in cards to give the same sort of thing we saw on the older server. I actually wound up putting the one-liners into <code> tags as they were already separated out by the cards and that gave them the same visual look as on the original hydra server (wrapping and even margins all around).

I think the only big looking difference is that the error stuff is now white on darker red instead of darker red on light red. You can checkout our hydra server as I've updated it to see what you think.

How would you guys want all these commits to merge them? Are you going to do a merge squash so it doesn't matter, or should I

  1. squash everything into the bootstrap update commit, or
  2. give better names to my fixup commits and leave it as is otherwise.

@twhitehead
Copy link
Contributor Author

By everything, I mean all the tweaks we have made to the bootstrap update commit. I would still leave the other javascript library update commits separate. I would of course list you guys as co-authors.

@samueldr
Copy link
Member

samueldr commented Apr 7, 2021

I wouldn't put the evaluation errors on red. This is because "Evaluation errors" is rather vague, and in Nixpkgs' case, things are fine even with the errors. So styling them the same exact way as other logs is probably better. @grahamc thoughts about the evaluation log?

@grahamc
Copy link
Member

grahamc commented Apr 8, 2021

I agree, the white on red is too intense. It seems like that style is optimized for very very short errors, but the evaluation error log should be optimized for reading.

w.r.t. commits: I'm inclined to do a squash and merge.

@twhitehead
Copy link
Contributor Author

I've gotten rid of the white on red, and expanded out the history on the two fixups commits I did, so that is everything I am aware of/wanted to do. I've updated our hydra sever with the latest as before.

Unless anyone else has anything else outstanding on this, I think it is clear to squash merge. Thank both of you very much for all the feedback, commits, and just general work pushing this one though. 👍

@grahamc grahamc merged commit 258b39f into NixOS:master Apr 9, 2021
@grahamc
Copy link
Member

grahamc commented Apr 13, 2021

Thank you so much for coming back to this PR and getting it across the line, @twhitehead!

@twhitehead
Copy link
Contributor Author

Thank you so much for coming back to this PR and getting it across the line, @twhitehead!

Not a problem at all. You guys made it super easy between the rebase and followup patches! 😁

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

Successfully merging this pull request may close these issues.

None yet

5 participants