Fixes SI-6170: issue with dragging scaladoc splitter over central iframe #1480
Conversation
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/715/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1425/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/715/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1425/ |
|
LGTM |
|
Hi guys- sorry I wasn't around to catch this. Though can you provide a bit more detail about the problem that has been fixed by this commit? On Safari, Chrome, and Firefox, what's in 2.10.x and 2.10.0-wip has a left panel that works totally fine for me. |
|
I am sure you are aware of the general problem with the splitter handle. You reverted jquery/layout to an older version here: #1405 to fix the issue. Unfortunately, older versions of jquery and newer browsers don't work very well together with SVG content, therefore breaking scaladoc diagrams. I don't know why it works for you, but it didn't for Vlad and me. Since having a recent jquery is desirable anyways, Vlad pushed #1463 and I offered my help to fix the splitter issue for real. The result of that being this PR, which includes the most recent jquery ui version (Vlad committed a css instead of the js in 1463 and I was a little trigger happy with my LGTM) and the single additional flag to make jquery layout and hence the splitter capture mouse events over iframes. Losing mouse capture over the central iframe was the underlying issue of the splitter being flaky. |
|
FYI, you'll find related discussions on the jquery layout list here: https://groups.google.com/forum/?fromgroups=#!searchin/jquery-ui-layout/$20center__maskContents |
|
Interesting, it works on Eugene's slightly outdated topic/reflection fork: https://dl.dropbox.com/u/166774/reflectiondocs/index.html#package |
|
Eugene's version uses jquery 1.4, which is hopelessly outdated and the one you reverted to to make the splitter work again. Again, it doesn't work well with SVG elements, at least not on newer browsers. The nightly doesn't work because the jquery-ui.js actually contains its CSS instead of the JS code, as I mentioned above. |
|
For the record, I don't know why the splitter works in conjunction with an older jquery version but not with a newer one. Maybe some spurious z-ordering issue but it could be anything given the number of hacks that go into those JS libs. I am not an expert and I don't care enough. Eventually, the link to the groups discussion should convince you that setting the maskContents flag is the way to go. Keeping an outdated jquery around, however, should not be an option. |
|
Well, stuff gets deprecated and removed in jquery as with anything else. And already once before, a new version of jquery broke part of Scaladoc. Javascript sucks in we don't get immediate feedback about what works and what doesn't when we update. This is how the splitter went into the distribution, broken, for several months- when kyzs updated jquery then, neither he nor Vlad or I seemed to experiment with the parts of scaladoc that touch jquery. Weird stuff happens. For example, earlier we had some weird issue with the iframes needing both a name and an id attribute to make something work (probably the splitter). At any rate, your proposed fix looks fine to me. I'd just rather that we generate the docs and test various parts of Scaladoc just to make sure that the new jquery didn't break anything more. |
|
We can use https://scala-webapps.epfl.ch/jenkins/view/scaladoc/job/scaladoc-diagram-nightly/ws/build/scaladoc/library/index.html for any scaladoc build we want. We just need to pass it the right git repo and branch. |
|
Why didn't you say this earlier, Heather? :) Do as many tests as you want, but it doesn't really have anything to do with this PR, does it? Vlad already committed the new jquery version in #1463. |
|
Oh, and the nightly docs are located at http://www.scala-lang.org/archives/downloads/distrib/files/nightly/docs-2.10.x/library/index.html#package (thanks to @lrytz for that!) |
|
Question, does this need to go in 2.10.0? It's a nice small change, but I'm not sure how to validate it. |
|
Yes it needs to go in. |
|
You have three options: you revert #1463, which will definitely break diagrams on some browsers. You keep #1463 but not this one, which fixes diagrams but definitely breaks the splitter. You also merge this one, which fixes the splitter, but might break something that we missed (just as any PR, I guess). |
|
As mentioned above- it just needs some manual testing and browsing though existing js to make sure all features still behave as expected in some major browser. |
|
Okay, than let's merge it and browse it. Why all the opposition? |
|
I think that's backwards. Let's browse it first, then merge. We're in
|
|
Ok, after testing it, seems ok. Didn't get enough time to figure out all of our versioning issues between plugins like I had intended, but this seems good. I'll add "organizing js-mess" to the queue for 2.10.1. |
|
I also tested Ingo's patch and everything works fine -- what's more, there are no more js-generated errors while navigating the pages. in RC-1 diagram highlighting doesn't work at all and there are cases when navigating through diagrams breaks the page initialization js and leaves buttons without events attached, which is really bad. So, from my perspective, merge merge merge, it's not scala code, it's not compiler code, and as far as my tests went, it fixes the bug and works flawlessly. Thanks @ingoem. So, it's up to @jsuereth to decide whether we merge or not. |
|
ok, it seems the risk has been assessed with enough confidence to let this go into RC2 |
|
If there is an RC2 I will merge it. Actually thought I had before. Otherwise, we'll save it for 2.10.1. |
|
Just for the record- Vlad's description above was based on him looking at Scaladoc on another branch. The bugs that he cites aren't accurate to what exists and is problematic in 2.10.0-wip. |
|
Not sure why we are still discussing this, but try click on a class in a diagram, you will be redirected to another page, but not the diagram. Sometimes the diagram doesn't even show up. Moreover, those arrow buttons on "linear supertypes" and alike sometimes don't work when navigating through the diagrams. All in 2.10.0-wip, because of the missing jquery-ui.js. |
|
Yep- and all of this is known :) (We use PRs as documentation- Vlad made some incorrect statements about On Wed, Oct 17, 2012 at 9:50 AM, Ingo Maier [email protected]:
Heather Miller |
|
I assume the problem I described is what Vlad means by diagram highlighting, because it tries to set a jquery ui highlight effect on the SVG when navigated to via a diagram. Which doesn't work. |
|
Nope- Vlad was talking about mouseover events and highlighting. I had the On Wed, Oct 17, 2012 at 10:05 AM, Ingo Maier [email protected]:
Heather Miller |
|
So, basically the problem this patch fixes is what Ingo described here:
My comment: s/sometimes/always/ |
That was never disputed. :) Once again- it's necessary to point out that the issues reported in your |
Yes, that can't be stressed enough: I was a deceivingly pointing everyone in the wrong direction with my issue descriptions. Josh should ban me from the scala community for 5 days for disciplinary reasons. And let's make 17th of October the 'Vlad inaccurately described a scaladoc issue' day. |
|
(I just noticed this thread while catching up with stuff happening in scala land) I want to support @heathermiller's efforts of documenting and clarifying stuff. Now us being distributed makes this very important. However, there's no reason for expulsion. |
|
We do have 5-day rocket trips for temporary expulsions. Could be fun. |
|
And on the way back you can just out of ship if you are up to beating a few world records! |
|
Visiting the ISS, beating Felix's record... I should write sloppy bug reports more often :) |
|
haha- Vlad, the first space-tourist sent on a company trip to the ISS! There's gotta be a first for everything... |
|
While Vlad is in space, let's merge this pull request as it has been signed off contingent on having an RC2. I think whether there's an RC2 is not a condition for merging into 2.10.0-wip, Thus, if we decide to promote RC1 to final (and hell freezes over), |
Fixes SI-6170: issue with dragging scaladoc splitter over central iframe
|
Elsewhere I already proceeded on the certainty of an rc2. |
Setting a simple jquery layout flag was enough. This PR also includes the correct jquery-ui.js. Review by @VladUreche, @heathermiller