>>> My Info: Win7_64, Nightly 53, 32bit, ID 20161219030207 (2016-12-19) STR_1: 1. Switch inspector to vertical mode (not equal to docking devtools to right side) 2. Click button to hide inspector sidebar AR: Sidebar resizes diagonally: to the bottom and right ER: Sidebar should resize normally: to the bottom This is a regression. Nightly 2016-05-26 is unaffected, Nightly 2016-12-19 is affected.

Honza: maybe you know what's happening here? This might be related to the new sidebar/splitter components, or some related changes done after that. The sidebar does hide correctly, but in vertical mode, it also expands weirdly to the right as it hides. Not a big deal, but this also might be an easy fix.

I did some analysis here: 1) The code responsible for side panel animation (not only in the Inspector but, also in Network and WebAudio panels, it's been also used be the old Debugger UI) is in ViewHelpers: https://dxr.mozilla.org/mozilla-central/rev/13603af3862d9583ed2feefb06e0988c2d7fed8c/devtools/client/shared/widgets/view-helpers.js#286 The problem seems to be the fact that both 'height' and 'width' attributes are set no matter if the panel should be animated horizontally or vertically (e.g. to the bottom). And I think it wasn't the case before refactoring (XUL -> HTML) so, this could be the breaking change. For the bottom animation the width attribute is definitely not necessary and if it's 0 the problem is fixed. We can pass custom and correct: width and height values through flags into the method and use it instead of the values from HTML attributes (if they are available). This was we could preserve back comp. @Julian: how does this sound to you? To sum, ViewHelpers.togglePane() method is used: 1) Old Debugger UI -> not needed anymore 2) Inspector panel 3) Network panel 4) WebAudio panel Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #2) > I did some analysis here: > > 1) The code responsible for side panel animation (not only in the Inspector > but, also in Network and WebAudio panels, it's been also used be the old > Debugger UI) is in ViewHelpers: > https://dxr.mozilla.org/mozilla-central/rev/ > 13603af3862d9583ed2feefb06e0988c2d7fed8c/devtools/client/shared/widgets/view- > helpers.js#286 > > The problem seems to be the fact that both 'height' and 'width' attributes > are set no matter if the panel should be animated horizontally or vertically > (e.g. to the bottom). And I think it wasn't the case before refactoring (XUL > -> HTML) so, this could be the breaking change. > > For the bottom animation the width attribute is definitely not necessary and > if it's 0 the problem is fixed. We can pass custom and correct: width and > height values through flags into the method and use it instead of the values > from HTML attributes (if they are available). This was we could preserve > back comp. > > @Julian: how does this sound to you? > The "width" and "height" are read from attributes programmatically set on the panel from inspector.js. So this acts kind of like flags already. We could set width to 0 when resizing in vertical mode (and potentially height to 0 when resizing in horizontal mode) This fixes the issue described in this bug, but introduces a regression with the following STRs: - resize inspector to vertical mode - collapse the sidebar - resize inspector to horizontal mode - expand the sidebar => no animation. Even when toggling in vertical mode, we need a correct negative margin-left/right in case we switch back to horizontal mode. The complete fix for this would be to : - set the margin-bottom _before_ the animation - set the margin-left/right _after_ the animation (and vice-versa when resizing in horizontal mode) I think this logic should be in view-helpers, so we'll need to test the impact in all panels mentioned above. I would also like view-helpers to measure the height/width needed for collapsing rather than forcing callers to set attributes on the element. Which means callers might have to tell view-helpers the direction of the toggling. A simpler fix is to add the following rule to inspector.css: > #inspector-splitter-box .vert .controlled { > width: 100%; > } This avoids negative margins having any impact when in vertical mode. I see you set good-first-bug on this, I'll set myself as mentor. Just a heads up, the easiest solution to implement is the CSS one by far, and much more suited to a good first bug :)

Got confused between horizontal and vertical :) the selector should be : > #inspector-splitter-box .horz .controlled

Can i work on this bug?

I would like to work on this bug.

George: I am assigning the bug to you if you are still interested! Sorry for missing your question earlier. You can use the "Need more information" box to make sure people get notifications about your question :) Some pointers to get started on devtools at https://wiki.mozilla.org/DevTools/Hacking or https://developer.mozilla.org/en-US/docs/Tools/Contributing I hinted at two solutions in comment 3, if you go for the complicated one, don't hesitate to ping me if you need any help! (Sorry amanaryan23, but since George asked first, I should check if they still want to work on this. You can find other good first bugs to get started with devtools on http://firefox-dev.tools/ . Sorry again!)

Thank you. I'm working on this. :)

Proceeded according to Method 1. Sorry I was late. I was busy in academics.

Comment on attachment 8837991 [details] [diff] [review] V1.patch Review of attachment 8837991 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't fix the issue, pay attention to comment 4 :) (my selector was wrong in comment 3) ::: devtools/client/themes/inspector.css @@ +214,4 @@ > width: 100%; > } > > +#inspector-splitter-box .vert .controlled { Let's add a comment above this new rule @@ +215,5 @@ > } > > +#inspector-splitter-box .vert .controlled { > + width: 100%; > + } nit: remove extra space

Comment on attachment 8843530 [details] [diff] [review] Final Patch Review of attachment 8843530 [details] [diff] [review]: ----------------------------------------------------------------- Selector is still incorrect, this should be > #inspector-splitter-box .horz .controlled Pay attention to the spaces. Did you test your patch locally? Also you did not address the comments from the previous review (adding a comment, removing the extra space before "}") ::: devtools/client/themes/inspector.css @@ +215,5 @@ > } > > +#inspector-splitter-box.horz.controlled { > + width: 100%; > + } remove the extra space before "}"