Closed Bug 1189492 Opened 4 years ago Closed 4 years ago

If Inspector has small width and is forced to switch from horizontal mode to vertical, then toggle-pane button stays visible (and operates wrong)

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 affected, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox42 --- affected
firefox47 --- fixed

People

(Reporter: arni2033, Assigned: jdescottes)

References

()

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(6 files, 3 obsolete files)

STR:   (tested on Win7, Nightly 42.0a1 (2015-07-29), fresh profile)
1. Open devtools->Inspector
2. Click "dock-window" button
3. Change Inspector's width and height so that it switched to vertical mode

RESULT:
Inspector in separate window has toggle-pane button always visible and it [the button] operates wrong.

EXPECTATIONS:
It should have no visible toggle-pane button *OR* (I like the second one better)
That button should hide the rules pane DOWN if Inspector is in vertical mode.
Oh, it's also reproducible if Inspector is opened as bottom toolbar (not as separate window), and the Firefox window has small width
Summary: Inspector in separate window has toggle-pane button always visible, even in vertical mode → If Inspector has small width and is forced to switch from horizontal mode to vertical, then toggle-pane button stays visible (and operates wrong)
Whiteboard: [DUPEME]
I have a feeling we already have this bug filed, but I can't find it.
Whiteboard: [DUPEME] → [polish-backlog]
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=easy]
Has STR: --- → yes
Duplicate of this bug: 1200104
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attached patch bug1189492.wip.patch (obsolete) — Splinter Review
This patch allows the togglePane method from ViewHelpers.jsm to collapse both horizontal and side panels. 

The switch between horizontal/vertical layouts is handled via media queries, and the only way I could find to allow this behavior was to force "0" margins from the css, using !important (either margin-left/right or top/bottom depending on the layout).

:vporof : Before going further, any feedback about the approach described above? (I feel like it's quite hacky so...)
Attachment #8719538 - Flags: feedback?(vporof)
Looks like the goal of this bug was only to hide the icon in horizontal mode. Implementing the horizontal collapsing seems to be for Bug 1200179.

Updating the title. My patch can be moved to Bug 1200179 depending on the feedback.
Summary: If Inspector has small width and is forced to switch from horizontal mode to vertical, then toggle-pane button stays visible (and operates wrong) → Inspector : hide "collapse panel" button when switching to vertical layout
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Looks like the goal of this bug was only to hide the icon in horizontal mode
That is wrong. The goal of this bug is to make this consistent and make a decision on that button.
Read the expectations. Bug 1200179 is duplicate of this.
Comment on attachment 8719538 [details] [diff] [review]
bug1189492.wip.patch

Review of attachment 8719538 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/widgets/ViewHelpers.jsm
@@ +244,5 @@
>  
> +    if (aFlags.visible) {
> +      // Hiding is always handled via margins, not the hidden attribute.
> +      aPane.removeAttribute("hidden");
> +    }

Why is this inside the conditional now? Doesn't it contradict the comment?

@@ +252,5 @@
> +
> +    let afterToggle = () => {
> +      if (!aFlags.visible) {
> +        aPane.setAttribute("hidden", "true");
> +      }

Oh I see now. Can you explain why we hide the panel once it's completely hidden?
Attachment #8719538 - Flags: feedback?(vporof) → feedback+
(In reply to Victor Porof [:vporof][:vp] from comment #7)
> Comment on attachment 8719538 [details] [diff] [review]
> bug1189492.wip.patch
> 
> Review of attachment 8719538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/widgets/ViewHelpers.jsm
> @@ +244,5 @@
> >  
> > +    if (aFlags.visible) {
> > +      // Hiding is always handled via margins, not the hidden attribute.
> > +      aPane.removeAttribute("hidden");
> > +    }
> 
> Why is this inside the conditional now? Doesn't it contradict the comment?
> 
> @@ +252,5 @@
> > +
> > +    let afterToggle = () => {
> > +      if (!aFlags.visible) {
> > +        aPane.setAttribute("hidden", "true");
> > +      }
> 
> Oh I see now. Can you explain why we hide the panel once it's completely
> hidden?

Thanks for having a look! And comments were misleading in the patch you reviewed, sorry about that.
Here's my current comment:

> 
> let afterToggle = () => {
>   if (!aFlags.visible) {
>     // set hidden to true to avoid transitions if the viewport width changes
>     // and the side-pane becomes an horizontal-panel (or vice-versa)
>     aPane.setAttribute("hidden", "true");
>   }
>   if (aFlags.callback) {
>     aFlags.callback();
>   }
> };
> 

We hide using margins in order to have a smooth collapsing transition.
When the viewport width crosses 700px, margins are updated because we want a different set of margins to be at 0 or at -{width/height}px. This forces the collapsed panel to appear for a short time.

The hidden attribute is only used here in order to prevent that. 

(and the misleading comment in the if section is gone too)
(restoring original title)
Summary: Inspector : hide "collapse panel" button when switching to vertical layout → If Inspector has small width and is forced to switch from horizontal mode to vertical, then toggle-pane button stays visible (and operates wrong)
If we support collapsing the inspector panel horizontally, the current icon could be confusing.

Helen: Modified the original debugger-expand.png and debugger-collapse.png to create "horizontal" versions. In this screenshot, from top to bottom:
- collapsed horizontal (new)
- expanded horizontal (new)
- expanded vertical (for reference)

Is this ok?
Attachment #8722967 - Flags: ui-review?(hholmes)
Attachment #8722967 - Flags: ui-review?(hholmes) → ui-review+
Comment on attachment 8722974 [details]
MozReview Request: Bug 1189492 - part3: show inspector toggle panel button in SIDE host;r=tromey

https://reviewboard.mozilla.org/r/36295/#review32899

Looks good to me, thanks.
Attachment #8722974 - Flags: review?(ttromey) → review+
Thanks for the review!

Investigating the try failures right now, could be linked to the fact that togglePane is now setting "hidden" to true.
Comment on attachment 8722972 [details]
MozReview Request: Bug 1189492 - part1: support horizontal collapsing in ViewHelpers togglePane;r=vporof

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36291/diff/1-2/
Comment on attachment 8722973 [details]
MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36293/diff/1-2/
Comment on attachment 8722974 [details]
MozReview Request: Bug 1189492 - part3: show inspector toggle panel button in SIDE host;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36295/diff/1-2/
Attachment #8719538 - Attachment is obsolete: true
Most try failures were linked to a rebase on a bad revision.
Some failures linked to setting hidden to true on the toggled pane: decided to revert this and simply remove the "animated" attribute once the transition is finished.
Comment on attachment 8722972 [details]
MozReview Request: Bug 1189492 - part1: support horizontal collapsing in ViewHelpers togglePane;r=vporof

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36291/diff/2-3/
Comment on attachment 8722973 [details]
MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36293/diff/2-3/
Attachment #8722973 - Attachment description: MozReview Request: Bug 1189492 - part2: add icon for inspector panel horizontal toggle;r=vporof → MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim
Attachment #8722973 - Flags: review?(vporof) → review?(ntim.bugs)
Comment on attachment 8722974 [details]
MozReview Request: Bug 1189492 - part3: show inspector toggle panel button in SIDE host;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36295/diff/2-3/
ntim: flagged you for review on patch2; here is a screenshot of the new icons in various states if you want to have a look before retrieving the patch.

Helen: after discussing with ntim, we decided to modify the existing icon and make it square. This way we can use it for both vertical and horizontal layout and don't have to create additional resources.

(this icon is only used in the inspector for now, but we can get rid of the current icon if you are ok).
Flags: needinfo?(ntim.bugs)
Attachment #8723845 - Flags: ui-review?(hholmes)
Attachment #8722967 - Attachment is obsolete: true
Comment on attachment 8722973 [details]
MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim

https://reviewboard.mozilla.org/r/36293/#review33347

::: devtools/client/jar.mn:352
(Diff revision 3)
> +    skin/images/toggle-pane-collapse.svg (themes/images/toggle-pane-collapse.svg)

While you're adding these, can you replace the old icons in other places as well ?

::: devtools/client/themes/images/toggle-pane-collapse.svg:1
(Diff revision 3)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

This icon looks a bit blurry on 1x screens, but I can publish a new version that fixes the problem.

::: devtools/client/themes/images/toggle-pane-expand.svg:1
(Diff revision 3)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

Same for this one.

::: devtools/client/themes/inspector.css:44
(Diff revision 3)
> -  list-style-image: url(images/debugger-collapse.png);
> +  list-style-image: url(chrome://devtools/skin/images/toggle-pane-collapse.svg);

How about just pane-collapse.svg and pane-expand.svg ?
Attachment #8722973 - Flags: review?(ntim.bugs)
The screenshot looks fine, although the icon is a bit blurry on Windows.
Flags: needinfo?(ntim.bugs)
Attached image pane-collapse.svg
Pixel snapped version of image
Attachment #8723979 - Attachment filename: file_1189492.txt → pane-collapse.svg
Attachment #8723979 - Attachment mime type: text/plain → image/svg+xml
Attached image pane-expand.svg (obsolete) —
I forgot to add a license header to pane-collapse.svg, so please don't forget to do it.
Attached image pane-expand.svg
That last SVG had a small issue, fixed it.
Attachment #8723982 - Attachment is obsolete: true
Comment on attachment 8723979 [details]
pane-collapse.svg

Making them square seems like a good idea, and the icons themselves look great.

Tim, I'd remove the width/height declarations personally—I don't understand why we're declaring them at the <svg> level anymore and not just within our CSS (makes the icons themselves more flexible in other places). What do you think?
Attachment #8723979 - Flags: feedback+
Attachment #8723845 - Flags: ui-review?(hholmes)
Comment on attachment 8722972 [details]
MozReview Request: Bug 1189492 - part1: support horizontal collapsing in ViewHelpers togglePane;r=vporof

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36291/diff/3-4/
Attachment #8722973 - Flags: review?(ntim.bugs)
Comment on attachment 8722973 [details]
MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36293/diff/3-4/
Comment on attachment 8722974 [details]
MozReview Request: Bug 1189492 - part3: show inspector toggle panel button in SIDE host;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36295/diff/3-4/
Comment on attachment 8722973 [details]
MozReview Request: Bug 1189492 - part2:inspector modify toggle icon to support horizontal layout;r=ntim

https://reviewboard.mozilla.org/r/36293/#review33645

Thanks!
Attachment #8722973 - Flags: review?(ntim.bugs) → review+
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #29)
> Comment on attachment 8723979 [details]
> pane-collapse.svg
> 
> Making them square seems like a good idea, and the icons themselves look
> great.
> 
> Tim, I'd remove the width/height declarations personally—I don't understand
> why we're declaring them at the <svg> level anymore and not just within our
> CSS (makes the icons themselves more flexible in other places). What do you
> think?

I don't agree, having the width/height attributes makes the images sized like where they are used, and it doesn't make things any less flexible, since you'd have to define dimensions in CSS both ways.
Comment on attachment 8722972 [details]
MozReview Request: Bug 1189492 - part1: support horizontal collapsing in ViewHelpers togglePane;r=vporof

https://reviewboard.mozilla.org/r/36291/#review33693
Attachment #8722972 - Flags: review?(vporof) → review+
Thanks for the reviews! Try is green, pushing.
https://hg.mozilla.org/mozilla-central/rev/4245d4c579be
https://hg.mozilla.org/mozilla-central/rev/e3ca5d7c28a9
https://hg.mozilla.org/mozilla-central/rev/0acb1e76bc20
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Duplicate of this bug: 1200179
Depends on: 1260053
Depends on: 1261972
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.