User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20160717030211 Steps to reproduce: 1. Start Nightly 2. Open DevTools > Inspector 3. Dock to side of Browser window 4. Resize devtools until portrait mode 5. Check sidebar and page toggle Actual results: The inspector sidebar is not displayed in portrait mode. And also the pane toggle does not work. You can expand sidebar manually by mouse dragging, but tab panel is blank. Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a8ab215f15d3a875f964663a6011b5a6cba67919&tochange=453c308dcab1e3236fde0c4ee2e6d0d3d2d60dae Expected results: The inspector sidebar should be displayed in portrait mode. And also the pane toggle works well.

Patrick, here is patch for the reported problem. Can I please ask for quick review, this is important to fix. Thanks! Honza

Comment on attachment 8772013 [details] [diff] [review] bug1287368.patch Review of attachment 8772013 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/inspector-panel.js @@ +479,5 @@ > this.sidebar.on("show", () => { > try { > sidePaneContainer.width = Services.prefs.getIntPref( > "devtools.toolsidebar-width.inspector"); > + sidePaneContainer.height = Services.prefs.getIntPref( We don't want the height to apply when the inspector is in horizontal mode, right? Also, we don't want the width to apply when the inspector is in vertical mode. Also, I think we should only have 1 pref: devtools.toolsidebar-width.inspector and use it for the width when the inspector is horizontal, and for the height when it is vertical. Maybe the devtools.toolsidebar-width.inspector name was not chosen carefully, but it doesn't matter much. We can use it because it already exists for our users, and just apply it where it makes sense. (note that horizontal is what you get when the toolbox is docked to the bottom, and vertical is what you get when the toolbox is docked to the side, unless its wide enough in which case it goes back to horizontal).

(In reply to Patrick Brosset <:pbro> from comment #3) > Comment on attachment 8772013 [details] [diff] [review] > bug1287368.patch > > Review of attachment 8772013 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/inspector/inspector-panel.js > @@ +479,5 @@ > > this.sidebar.on("show", () => { > > try { > > sidePaneContainer.width = Services.prefs.getIntPref( > > "devtools.toolsidebar-width.inspector"); > > + sidePaneContainer.height = Services.prefs.getIntPref( > > We don't want the height to apply when the inspector is in horizontal mode, > right? > Also, we don't want the width to apply when the inspector is in vertical > mode. Agreed on applying both on IRC. > Also, I think we should only have 1 pref: > devtools.toolsidebar-width.inspector > and use it for the width when the inspector is horizontal, and for the > height when it is vertical. > > Maybe the devtools.toolsidebar-width.inspector name was not chosen > carefully, but it doesn't matter much. We can use it because it already > exists for our users, and just apply it where it makes sense. > > (note that horizontal is what you get when the toolbox is docked to the > bottom, and vertical is what you get when the toolbox is docked to the side, > unless its wide enough in which case it goes back to horizontal). Patrick, I know you don't like too many prefs, but width (in landscape mode) and height (in portrait mode) are two different values. If we store it into one pref and overwrite the previous value we force the user to readjust the splitter size every time the orientation is switched and adjusted. Note that switching can happen automatically in the portrait mode when the use changes width of the Toolbox. I think that having two prefs is valid in this case (besides that handling additional dock/undock events for pref syncing will make the code more complex). Anyway, I can see following options: 1) We could store width and height into one pref (as a string e.g. `400,400`). But, this requires creating a new pref since the current one has type integer. Back comp not preserved. 2) We do not store the height of the sidebar in the portrait mode and use only default. This is how it works now (but it feels incomplete). 3) We store width and height into two independent prefs. Back compatibility preserved. Later (as soon as bug 1260552 is fixed) we wrap the logic within the new HTML ToolSidebar() so, it's transparent when used in other panels. Honza

Alright, you made a good point that it's very easy to switch from horizontal to vertical in side docking mode just by resizing the toolbox. I guess that this may happen more often than switching docking position, and therefore it may be frustrating more often to users to have to move the splitter. So let's go with (3).

Comment on attachment 8772013 [details] [diff] [review] bug1287368.patch Great! Honza

Comment on attachment 8772013 [details] [diff] [review] bug1287368.patch Review of attachment 8772013 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/inspector-panel.js @@ +485,5 @@ > } catch (e) { > // The default width is the min-width set in CSS > // for #inspector-sidebar-container > sidePaneContainer.width = 450; > + sidePaneContainer.height = 450; Please add a comment above these 2 lines that says that only one of these values at a time is really useful, but setting both doesn't break anything. Such a comment may be useful when we migrate the splitter to HTML, since we might need to change this.

(In reply to Patrick Brosset <:pbro> from comment #7) > Please add a comment above these 2 lines that says that only one of these > values at a time is really useful, but setting both doesn't break anything. > Such a comment may be useful when we migrate the splitter to HTML, since we > might need to change this. Done Thanks! Honza

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/081628fb2043 Fix inspector sidebar portrait mode; r=pbro

backed out since this might have caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10660615&repo=fx-team

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/e7cd99539c53 Backed out changeset 081628fb2043 for dt tests failures

This is weird, here is another try push with the same patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7870ddc5e04 Honza

Any chance to get a screenshot? (when the test failed) Honza

Honza: you can have links to screenshots in the detailed logs for each job. For instance: - detailed log: https://treeherder.mozilla.org/logviewer.html#?job_id=24219588&repo=try#L8067 - screenshot: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/b595c502f2ac982d07733d49a687a4269ed7c904f84e39136d8cec826ef3d4cfa23d3995d424f4cd7e9434a7acbee494c0c39d326acf7d526586a4341394305b

Ah, I see it now, nice! Honza

New patch attached. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71110976cffe Honza

NI=me

Try looks good, problem fixed. Honza

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/762845e3c684 Fix inspector sidebar portrait mode. r=pbro

Verified with latest Nightly 50.0a1, across platforms [1], but there is still a potential issue: 1. While docked to bottom, collapse CSS pane 2. Switch to portrait mode and drag up the CSS pane Result: The toggle button points to Expand the pane → https://i.imgur.com/Z6hGBQt.png. Note that this behavior is present also on 49.0a2. Honza, any ideas? [1] Windows 10 64-bit, Ubuntu 16.04 64-bit

I reproduced this bug using Fx 50.0a1, build ID: 20160717030211, on Windows 10 x64. I can confirm the bug is fixed, I verified using Fx 50.0a2 build ID:20160807004000 , on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11.6 .

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #21) > Verified with latest Nightly 50.0a1, across platforms [1], but there is > still a potential issue: > 1. While docked to bottom, collapse CSS pane > 2. Switch to portrait mode and drag up the CSS pane > Result: The toggle button points to Expand the pane → > https://i.imgur.com/Z6hGBQt.png. Note that this behavior is present also on > 49.0a2. > Honza, any ideas? Nice catch! It shouldn't be possible to 'drag up' the panel and this is covered in bug 1171449 Honza