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

Allow more usage of jbrowse without iframe #844

Merged
merged 53 commits into from Apr 10, 2018

Conversation

Projects
None yet
5 participants
@cmdcolin
Contributor

cmdcolin commented Jan 24, 2017

This PR adds more specificity to the CSS used by jbrowse to allow it to be loaded better in other pages without iframe

Some notable changes

  • The "jbrowse" CSS class was prefixed to almost all css styles
  • The root styles from src/dojo/resources/dojo.css were removed, which added global fonts. It seems this stylesheet is not essential though, and it is called a "baseline file for general usage" and that it is not the "foundation for more complex things", so I think it's not essential style
  • The font/font-family that was added via css/main.css was modified so that it only applies to a "jbrowsecontainer" class, which is added to the GenomeBrowser div
  • The width/height 100% has to be applied to "html, body" in css/main.css, because otherwise jbrowse comes up blank. I don't fully understand the purpose of having to set width/height 100% on "html, body" but it doesn't seem to have consequences for embedding (might need verification)

This PR adds a test page tests/drupal.htm to the test directory which you can view the embedded page in drupal like this

Example with volvox data
http://localhost/jbrowse/tests/drupal.htm?data=http%3A%2F%2Flocalhost%2Fjbrowse%2Fsample_data%2Fjson%2Fvolvox&loc=ctgA%3A26209..35186&tracks=DNA%2Crnasim%2CGenes%2CCDS%2CExampleFeatures%2CEST%2Cvolvox_gtf%2Cvolvox_gff3_tabix%2Cvolvox_bed_tabix&highlight=

Example with the "error page"/Congratulations, jbrowse is on the web... message
http://localhost/jbrowse/tests/drupal.htm

xref here for drupal examples #777

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jan 24, 2017

Some screenshots of plain drupal page, drupal page with the jbrowse error message, and with genome browser content

screenshot-localhost-2017-01-24-10-07-48 crushed
screenshot-localhost-2017-01-24-10-07-42 crushed
screenshot-knowpulse usask ca-2017-01-24-09-53-48 crushed

And then the bad screenshot with the jbrowse about box for example

screenshot-localhost-2017-01-24-10-10-01 crushed

The dialog boxes in the html appear outside the genomebrowser div, so this issue is not entirely unexpected, but maybe it can be fixed somehow

margin: 0.5em 0 1em 0.3em;
}
div.textfilter input {
.jbrowse div.textfilter input {
padding: 2px 0 2px 18px;
width: 100%;
height: 100%;

This comment has been minimized.

@nathandunn

nathandunn Jan 25, 2017

Contributor

Is there a reason to remove height: 100%

If I load something like this:

<div id="jbrowse1">
<script>
$( "#jbrowse1" ).load( "/jbrowse/index.html", function( response, status, xhr ) {
});
</script>

I get a very tall "filter tracks"

image

This comment has been minimized.

@nathandunn

nathandunn Jan 25, 2017

Contributor

@cmdcolin @enuggetry @laceysanderson I do realize this wasn't the intended use-case, but I wanted to make sure it works in a few different scenarios. Either way this is awesome.

This comment has been minimized.

@cmdcolin

cmdcolin Jan 26, 2017

Contributor

It looks like this "tall filter tracks" thing would happen on master branch too if you used jquery load(). That would be interesting to fix I suppose, but isn't specific to this branch

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Jan 25, 2017

Tested and the links seem to work great.

@laceysanderson

This comment has been minimized.

laceysanderson commented Jan 26, 2017

I don't have a dev. environment set up but based on the Drupal screenshots @cmdcolin attached it all looks good as far as Tripal is concerned. It even looks like the dojo issue has been fixed and the page font is now un-altered 👍

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jan 26, 2017

I had added a couple things since that last discussion on #777 to allow fonts to be specified more specifically in the jbrowse div, so it should allow allow fonts outside the genome browser to be customized freely

nathandunn referenced this pull request in alliance-genome/agr_archive_initial_prototype Feb 22, 2017

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 8, 2017

BTW, @enuggetry tested with Apollo and it worked great. There were some other issues to do with library collision, but that was another matter.

@enuggetry

This comment has been minimized.

Contributor

enuggetry commented Mar 8, 2017

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 8, 2017

@enuggetry The only problem I'm running into right now (I almost have Apollo working with JBrowse as a div) is that I have two different versions of jQuery. Hopefully, that is something that could be installed via bower / npm, as well. I'd hate to have to manually install that as well. Otherwise, this is still an improvement.

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 10, 2017

I think the concept we are looking at now is running multiple organisms off of the same JBrowse, but never having to reload the rest of the Apollo screen (and potentially not using an iFrame). As such, I think that Ajax might be a way to do this . . . but we'll see.

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Mar 10, 2017

Changing organisms without a refresh is somewhat tricky but probably doable. The "Genome->Open sequence" does that for example. It has to do some tricks to make it work like the teardown method in Browser.js. It might be different if you're doing it from jquery externally though.

Anyways, the css problem is sort of complicated. The css isolation from this pr goes some ways, but even then it doesn't stop some external styles from getting into the jbrowse div (and things like css resets on a specific div don't seem to work well)

A total alternative to this PR is to look into web components. It is promising but it uses things like shadow DOM which is sort of new. I looked into it one time but it's sort of hard to make it work on something complicated like jbrowse.

Anyways, just some thoughts

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 10, 2017

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Mar 10, 2017

Yep. just wanted to make sure to emphasize that

Note that if you are not getting large amount of css collisions from the wrapper page, you can just get embedded iframe-less jbrowse working pretty well by putting relevant bits of the normal (e.g. master branch) index.html into your page, so you do not necessarily need this PR.

The tripal example was sort of extreme because it was generating large amounts of css collisions (css collision might just be a term I made up , but I use it to refer to some style that something like tripal normally uses being applied into jbrowse div or jbrowse styles also affect the tripal page). That is what motivates this PR largely is to avoid those "conflicts".

there are a number of sites that do this iframe-less embedding already like CoGe I think

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 10, 2017

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Mar 10, 2017

Hmm. Technically jbrowse core does not use jquery, so that might be something in the jslib folder of the apollo client and whichever other one is used

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Mar 16, 2017

I think that jQuery is used in a lot of the menu stuff. Either way, it should be built using bower like the other dojo source packages.

@laceysanderson

This comment has been minimized.

laceysanderson commented May 31, 2017

What's the status on this? I would really like to see it make it's way into a release soon...

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Jun 1, 2017

@laceysanderson I think this is ready. My issues were unrelated and shouldn't prevent any issues.

@cmdcolin / @enuggetry Will the CSS changes cause any collisions or delays?

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jun 1, 2017

I think the PR is in a little better state now.

The dialogs are now styled correctly which was a little tricky since dialogs (and menu items, maybe even other things) are created outside the GenomeBrowser div so that complicates trying to contain all styles just inside the GenomeBrowser div

But in any case, the PR should be in an ok state. Testing welcome on it

@rbuels

This comment has been minimized.

Collaborator

rbuels commented Jan 23, 2018

What happened to this? Is it still alive or needed @cmdcolin?

@laceysanderson

This comment has been minimized.

laceysanderson commented Jan 23, 2018

We definitely still need it, for what that's worth. Beyond that, I don't know about status

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Jan 23, 2018

@rbuels I think its probably fine. My only worry is adding the additionally scoped class definition might break something further down, but I'm guessing it doesn't. I think I'd tested this with Apollo before, though, and it was fine. Maybe a few more tests and we are good to go?

@nathandunn

This comment has been minimized.

Contributor

nathandunn commented Jan 23, 2018

I really like having the more specifically scoped CSS classes.

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Jan 24, 2018

If the interest for this PR exists then that is cool, but I think it could use manual testing still. There are just a lot of css changes though so it's hard to really verify both just in standalone jbrowse and in the new context of embedding

Some remaining issues I can see, issue can be in progress for time being I suppose

  • mouseovers aren't necessarily working with SNPCoverage (vertical bar not appearing)
  • some buttons don't have their icons (dropdown triangle on track labels for example)
  • the code for coloring bases on SNPCoverage/Alignments2 tracks is failing if crossdomain stylesheets are included (firefox 58, worked in 57). this is because the "try/catch" in the colorForBase producing an exception. This issue is not strictly related to this PR but it is obvious when you view the drupal example that includes external stylesheets.

@nathandunn nathandunn changed the base branch from master to dev Feb 7, 2018

@rbuels

This comment has been minimized.

Collaborator

rbuels commented Apr 8, 2018

@cmdcolin I've fixed all the issues I've come across. It's still adding a tundra class to the document body, but I don't think there's any way to avoid that, because of the dijit modal dialogs.

Could you poke around with me and look for other problems?

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Apr 8, 2018

It's looking great :) drupal example seems to be working again

Here are a couple observations

  • The SNPCoverage/Alignments2 base coloring not working on drupal example
  • The style of the drupal page gets into the genotypes dgrid table in VCF popup
  • The hover score on SNPCoverage/Wiggle gets styled using outside styles
  • I made the file_dialog get imported outside of the .dijitDialog scope because it had additional .fileDialog scope, but that is probably not right solution.
@rbuels

This comment has been minimized.

Collaborator

rbuels commented Apr 9, 2018

@cmdcolin nice catches. I think I've fixed those, could you QA it again?

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Apr 9, 2018

New issue seems like the variant dialog is very wide in full screen view

screenshot-localhost-2018 04 09-16-44-32

@cmdcolin

This comment has been minimized.

Contributor

cmdcolin commented Apr 9, 2018

  • The ? icon in the About this track and Help->About menu is empty
  • The Help->General tab is blank for some content
  • "View->Resize quantitative tracks" icon empty
  • Fonts are slightly smaller and have less padding than in previous versions in dijit menus
  • Open and cancel buttons are not centered in popups
  • The vertical bar for mouseover on Wiggle and SNPCoverage not showing up when in drupal example (works on full screen view)

Some minor issues there but if you have ideas for fixes that would be awesome :)

@rbuels rbuels merged commit d01567c into dev Apr 10, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@rbuels rbuels deleted the css_isolation branch Apr 10, 2018

@laceysanderson

This comment has been minimized.

laceysanderson commented Apr 10, 2018

Yay!! 🎉 Thank you! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment