1094466 - Similar jobs panes scroll together

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P4)

Description

[Steve Fink [:sfink] [:s:]]

When there is a long list of similar jobs and you scroll down to the bottom and click on one, the right pane will remain empty because it scrolls with the job list. It would be better if they were separate, so you could click through a bunch of jobs and immediately see the detail info in the right pane.

Comment 1

[Johannes Pfrang [:johnp]]

Attachment: Pull Request

Comment 2

[Ed Morley [:emorley]]

Comment on attachment 8567150 [details] [review]
Pull Request

A "review?" without a reviewer can get lost in the cracks - assigning the review to Cameron, since I trust him more than me for this PR :-)

Comment 3

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/a218a9959bde4cd164d82a76f45a65388892fdff
Bug 1094466 - Separate scroll-panels for similar jobs panel
This patch gets rid of overflowing for the bottom header and propagates down the content heights in a way, that the plugins can have proper scroll panes.
If we wouldn't explicitly inherit the height, it'd get calculated by its children, and therefore the scroll pane would overflow.
The change to pluginpanel.html is needed because otherwise we would have multiple div's with a explicitly set height, although their contents are display:none.

https://github.com/mozilla/treeherder-ui/commit/b0190967168d1ad152d0a08bace4a4fd1e5403cf
Bug 1094466 - Fix Chrome compatibility and other things
Make it compatible with Chrome by specifying flex-properties everywhere and let them handle the sizing.
Description: https://stackoverflow.com/questions/20959600/height-100-on-flexbox-column-child (we just have a couple more divs)
Chrome Issue: https://code.google.com/p/chromium/issues/detail?id=341310
Because this had to be extensively tested in Responsive Design View, there are also some other changes related to proper resizing/positioning of certain elements.

https://github.com/mozilla/treeherder-ui/commit/79fb7a67901ba262ab9c12dd02e1b4dbccced89d
Merge pull request #382 from johnp/bug-1094466-fix-similar-pane-scroll-bars

Bug 1094466 - Separate scroll-panels for similar jobs panel

Comment 4

[William Lachance (:wlach)]

I'm going to reopen this one. The > * > * style rules, combined with the fact that they're applied against an id (bottom-center-bottom) has unintended bad side effect sometimes, see bug 1149687.

Comment 5

[William Lachance (:wlach)]

Possible ways to fix this:

1. Figure out a way of getting the same behaviour without the bulk '* > * >' rules (not sure if this is possible, but it would be preferable since it would I think result in us seeing less surprises in the future). The rules are here: https://github.com/mozilla/treeherder-ui/blob/master/webapp/app/css/treeherder.css#L763
2. Turn the bottom-center-bottom (and probably all related items) into classes rather than id's. This would allow us to override them easily, without using hacks like "!important"

Comment 6

[Johannes Pfrang [:johnp]]

This was an expected outcome I noticed while writing the PR.[1] I couldn't find a way to implement your point 1 because of bugs in the CSS Implementation in Chrome & Firefox (see the PR for more information).

Point 2 would be nice, but the problem here is passing down the height through the AngularJS ng-repeat/ng-include, because those don't have per-tab IDs/classes.

There's also another problem (and also something where the Chrome & Firefox implementations do different things): According to my understanding of the flex-box CSS specification, the height/width of a parent isn't inherited if the flow-direction changes, but IIRC, Chromium does inherit it both ways (IMHO against the spec, but I could be wrong). Our out-of-date CSS flex-box implementation doesn't do any height-clamping (Bug 1000957) in this case.

Unfortunately, because of a Chromium-Bug [2], we cannot pass down the height any other way (I know of).

I'd very like to get rid of the flex-properties, because they are just too unreliable for us ATM, but I couldn't find another way to fix Chrome. btw, point 2 won't help us, because by overriding anything we'll likely loose the correct height.

Anyway, it's nearly 1 AM here and I have to get up early. I'm not sure if I can get to work on this in the next couple of days. Probably I could spend some time on it on the weekend, but if you have fresh ideas on how to resolve this issues, feel free to grab it ;)

[1] https://github.com/mozilla/treeherder-ui/pull/382#issuecomment-76013475
[2] https://code.google.com/p/chromium/issues/detail?id=341310

Comment 7

[William Lachance (:wlach)]

So, I guess maybe I don't understand why we need to specify height at all (with a few exceptions, I don't think setting height in CSS is a good plan). What problem are we trying to solve here? If we have two panes, and they need to scroll separately, can't we just fit them into a horizontal flexbox and set the overflow properties on their contents? Sorry if I'm missing something obvious. 

I also don't see what the difference is between specifying a CSS property on and id versus a class. From what I understand, the inheritance rules should be the same, it's just that the latter is easier to override.

To be clear, this isn't an urgent drop-everything issue. Things are more or less working and I'm happy to leave the patch in until we have a better fix. I just want to make sure that things are maintainable over the long term.

Comment 8

[Johannes Pfrang [:johnp]]

tldr: Complicated issue; IHMO the 'best fix' would be, if the Chromium team fixes their bug, so we can use 'height:100%' like in my first commit. I fully agree with your 'maintainability' point, and I was quite reluctant at first to go this way, but, after running into bugs in two browser implementations, it was the only way remaining. Also, for some words to 'temporary fix', look at the end of this comment.


Longer step-by-step explanation (I hope I didn't get something wrong):

#bottom-center-panel has a dynamic height, resizable by the #bottom-panel-resizer.

It's children are:
#bottom-center-top has a fixed (min-)height of 33px.
#bottom-center-bottom (max-)height is therefore calc(100% - 33px).

The target of this bug is, to get this calculated height to the .similar_jobs div, which is a horizontal flexbox with the overflow properties set on it's two content-divs (.left_/.right_panel), just like you suggest in §1.

This is the way the height has to get to the tab-panels:
#bottom-center-bottom -> |ngRepeat| (*4) -> |ngInclude| -> |PluginController| (e.g. similar_jobs)
If anyone has another idea on how to get the same height in the first and the last div mentioned here, please share ;)

Now, #bottom-center-bottom has i.e. 4 children (ngRepeat; one for each tab). Each one must inherit the full height of the parent, so it's height is neither shared with the other children (shouldn't happen either way, because they are all, apart from the selected one, display:none now), nor it is calculated by it's content.

In Firefox that's no problem, because just setting 'height: 100%' correctly inherits the full height of the parent. This is pretty much what the first commit in my pull request does. [1]

In Chromium this doesn't work, because of the linked bug. The height isn't inherited, but rather treated as 'auto' and in turn is sized by it's content. Just remove all of the '> *'... rules in the Inspector and replace them with one 'height: 100%'. Firefox will still work, while Chrome looses the height of the #bottom-center-bottom in the e.g. .similar_jobs div (or any other controller).

Therefore my plan was to use 'display:flex' to pass down the heights with the '> *'... rules.
I tried to only use this Chromium-specific fix for Chrome, so I used the vendor prefixed versions. Unfortunately, those didn't work together with the original 'height: 100%' approach in Chrome (but in FF), so I had to remove the original approach and focus on the one that worked in both browsers.

This was then done with the #bottom-center-bottom[...] changes in my second commit. [2]

[1] https://github.com/johnp/treeherder-ui/commit/a218a9959bde4cd164d82a76f45a65388892fdff
[2] https://github.com/johnp/treeherder-ui/commit/b0190967168d1ad152d0a08bace4a4fd1e5403cf


Back to the talos-panel issue: I had a short look at the temporary fix, and I'm pretty sure it truncates the talos-tabs content if it overflows. Just adding an 'overflow:auto' to the .talos-panel should do. I'm not sure why I didn't add that div, because there's no PluginController in the talos-tab, and that is the reason why the '> * > * > *' messed it up... But it's nice to have something like that there now. I'll have a look at it, once the current fix live on allizom.

Comment 9

[William Lachance (:wlach)]

I still don't understand why it is necessary to set the height to 100%. If you're inside a flexbox container, you should be able to set flex to "auto", and the element should just consumer the remainder of the available space. See for example:

https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Using_flexbox_to_lay_out_web_applications#Example_2.3A_Flowing_a_set_of_containers_vertically

Won't the elements in bottom-center-bottom just flow naturally inside such a container, depending on their actual height?

Comment 10

[Johannes Pfrang [:johnp]]

I used the code in your link and modified it to show one part of our situation:

https://jsfiddle.net/2rhtx6m0/

Interpret 'flexible size' as 'bottom-center-bottom' and open the link in Firefox and in Chrome. Our CSS flex-box implementation is out of date and does not clamp the height if the 'flex: auto' element to the height of the flexbox container. Chrome does the right thing here and clamps the height.

See bug 1000957#c4 for another nice jsfiddle showing the problem with an explanation from :dholbert.

Comment 11

[Johannes Pfrang [:johnp]]

Also, fwiw, the 'height: 100%' was just used in my first approach and has been deleted with the second commit. The workaround for the Firefox bug with the flex-approach of the second commit was specifying an explicit height on the flexbox container (using max-height).

Comment 12

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/6195d364e04d1f63fdd37ee83215807423424eca
Bug 1094466 - Separate scroll-panels for similar jobs panel
This patch gets rid of overflowing for the bottom header and propagates down the content heights in a way, that the plugins can have proper scroll panes.
If we wouldn't explicitly inherit the height, it'd get calculated by its children, and therefore the scroll pane would overflow.
The change to pluginpanel.html is needed because otherwise we would have multiple div's with a explicitly set height, although their contents are display:none.

https://github.com/mozilla/treeherder/commit/a47eeeda1a41a4f90c9eadc524e103f0d69643fe
Bug 1094466 - Fix Chrome compatibility and other things
Make it compatible with Chrome by specifying flex-properties everywhere and let them handle the sizing.
Description: https://stackoverflow.com/questions/20959600/height-100-on-flexbox-column-child (we just have a couple more divs)
Chrome Issue: https://code.google.com/p/chromium/issues/detail?id=341310
Because this had to be extensively tested in Responsive Design View, there are also some other changes related to proper resizing/positioning of certain elements.

https://github.com/mozilla/treeherder/commit/bc2413d93e6617320168cc66f2a9d5e392fef150
Merge pull request #382 from johnp/bug-1094466-fix-similar-pane-scroll-bars

Bug 1094466 - Separate scroll-panels for similar jobs panel

Comment 13

[Jonathan French (:jfrench)]

It sounds like this bug is Fixed based on the above merges yesterday, so I'll mark it so unless anyone objects.

Comment 14

[William Lachance (:wlach)]

(In reply to Jonathan French (:jfrench) from comment #13)
> It sounds like this bug is Fixed based on the above merges yesterday, so
> I'll mark it so unless anyone objects.

If you look at the history, I reopened this because I was not completely happy with the solution (it caused other problems). The above comments were just triggered by Ed's ui->service merge yesterday.

Still, I can't really find a fault in Johannes' reasoning above, so I guess perhaps we *should* just mark this one as fixed. Hopefully at some point browsers will evolve to make a better solution possible...

Comment 15

[Jonathan French (:jfrench)]

Gah. I am fine with it being reopened if needed in that case :)