Closed Bug 1287368 Opened 3 years ago Closed 3 years ago

The inspector sidebar is not displayed in portrait mode

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: Honza)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 1259819
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [devtools-html] [triage]
Duplicate of this bug: 1287415
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Attached patch bug1287368.patch (obsolete) — Splinter Review
Patrick, here is patch for the reported problem. Can I please ask for quick review, this is important to fix.

Thanks!
Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8772013 - Flags: review?(pbrosset)
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
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).
Attachment #8772013 - Flags: review?(pbrosset)
(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
Flags: needinfo?(pbrosset)
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).
Flags: needinfo?(pbrosset)
Comment on attachment 8772013 [details] [diff] [review]
bug1287368.patch

Great!

Honza
Attachment #8772013 - Flags: review?(pbrosset)
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.
Attachment #8772013 - Flags: review?(pbrosset) → review+
Attached patch bug1287368.patch (obsolete) — Splinter Review
(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
Attachment #8772013 - Attachment is obsolete: true
Attachment #8772322 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/081628fb2043
Fix inspector sidebar portrait mode; r=pbro
Keywords: checkin-needed
backed out since this might have caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10660615&repo=fx-team
Flags: needinfo?(odvarko)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e7cd99539c53
Backed out changeset 081628fb2043 for dt tests failures
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
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
Flags: needinfo?(odvarko) → needinfo?(cbook)
Ah, I see it now, nice!

Honza
Flags: needinfo?(cbook)
Attached patch bug1287368.patchSplinter Review
New patch attached.

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

Honza
Attachment #8772322 - Attachment is obsolete: true
Attachment #8772883 - Flags: review+
NI=me
Flags: needinfo?(odvarko)
Try looks good, problem fixed.

Honza
Flags: needinfo?(odvarko)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/762845e3c684
Fix inspector sidebar portrait mode. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/762845e3c684
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(odvarko)
QA Contact: adalucin → cristian.comorasu
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 .
Status: RESOLVED → VERIFIED
(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
Flags: needinfo?(odvarko)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.