Closed Bug 1247729 Opened 4 years ago Closed 3 years ago

Move the box model panel into the computed styles panel

Categories

(DevTools :: Inspector: Computed, defect, P2)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: hholmes, Assigned: gl)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [btpp-fix-later])

Attachments

(2 files, 8 obsolete files)

The box model probably doesn't need its own panel. As its own panel, it contributes to devtools noise where I think there doesn't need to be. With five panels already enabled in the the inspector-rules area, I think it would be a good idea to fold the contents of the box model panel into the computed styles at the bottom.
Long ago, I filed a lot bugs for potential improvements to the box model view: bug 1150496.
My intention was I thought the box model could become a lot better in explaining why a certain element appeared the way it did and where it did. And doing so required showing more information in and around the box model diagram.

I don't think moving the box model into the computed-style panel is incompatible with these improvements, so I'm not against this move. Plus, we do have too many tabs right now.

Maybe bgrins and gl have ideas about this.
Flags: needinfo?(gl)
Flags: needinfo?(bgrinstead)
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #0)
> The box model probably doesn't need its own panel. As its own panel, it
> contributes to devtools noise where I think there doesn't need to be. With
> five panels already enabled in the the inspector-rules area, I think it
> would be a good idea to fold the contents of the box model panel into the
> computed styles at the bottom.

Sure, that makes sense.  The computed panel already shows the computed value of these properties anyway.  The main downsides I can think of are that:
1) It will require some scrolling to get to (especially if 'browser styles' is enabled).  Although that's more accessible than clicking on a different tab.  The main annoyance would be if you are toggling between elements to compare their box model and you had to keep scrolling to the bottom each time.  That could be solved maybe by moving it to the top of the computed view, although that brings up issues with the filter box's positioning I think.
2) It might not as be discoverable since it's not surfaced in the top level UI

I'm all for reducing the number of tabs though.  Curious if anyone has thoughts on (1).  To further illustrate: I've had times where I know some element is really tall / causing overflow but don't know which element is so tall and which one is shorter and overflowing.  So I'll start at the bottom of the DOM tree and press 'up' continually while keeping an eye on the box model panel until it changes how I'm expecting.  If it were located at the bottom of the computed view, it will likely scroll out of view if one element has more computed styles listed than another.  In fact any movement at all would make it hard to keep an eye on the value I care about.
Flags: needinfo?(bgrinstead)
If we move it into the computed view, we should do what chrome is doing and move it to the top and shift the filter bar under the box model.

I also don't think it would be an idea to move the box model to also be under the rule view.
Flags: needinfo?(gl)
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> If we move it into the computed view, we should do what chrome is doing and
> move it to the top and shift the filter bar under the box model.

That would change the convention we have for filter bars being pinned to the top as the content behind it scrolls (like in rule view and font inspector).  Also UI-wise it might be a little tricky given the controls are in a secondary toolbar that would just be floating around.  But yeah, moving it to the top would give it a fixed position which would resolve my concern.

Maybe it could be below the filter bar but somehow collapsible so that people could hide it if they didn't care about it?
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> > If we move it into the computed view, we should do what chrome is doing and
> > move it to the top and shift the filter bar under the box model.
> 
> That would change the convention we have for filter bars being pinned to the
> top as the content behind it scrolls (like in rule view and font inspector).
> Also UI-wise it might be a little tricky given the controls are in a
> secondary toolbar that would just be floating around.  But yeah, moving it
> to the top would give it a fixed position which would resolve my concern.
I agree, I like this idea.

> Maybe it could be below the filter bar but somehow collapsible so that
> people could hide it if they didn't care about it?
I'm not totally certain I understand the concern re: the filter toolbar. Brian, could you elaborate?
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #5)
> > Maybe it could be below the filter bar but somehow collapsible so that
> > people could hide it if they didn't care about it?
> I'm not totally certain I understand the concern re: the filter toolbar.
> Brian, could you elaborate?

I mean that the filter bar lives inside a little sub-toolbar that is pinned directly below the tabs.  If we were to move it below the box model somehow, then (a) it might visually look weird to have that toolbar floating in between the box model thing and the rest of the rows.  And (b) the filter would scroll out of view instead of being pinned to the top, which is different behavior than our current filtering in rule view / font view.  Does that make sense?
See Also: → 1249569
How about making the box model fixed at the top and make its display togglable like the pseudo-classes panel?
The only issue I can see with this is that it's taking vertical space from the list of computed values.

Sebastian
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #5)
> > > Maybe it could be below the filter bar but somehow collapsible so that
> > > people could hide it if they didn't care about it?
> > I'm not totally certain I understand the concern re: the filter toolbar.
> > Brian, could you elaborate?
> 
> I mean that the filter bar lives inside a little sub-toolbar that is pinned
> directly below the tabs.  If we were to move it below the box model somehow,
> then (a) it might visually look weird to have that toolbar floating in
> between the box model thing and the rest of the rows.  And (b) the filter
> would scroll out of view instead of being pinned to the top, which is
> different behavior than our current filtering in rule view / font view. 
> Does that make sense?

Ah, I see. I agree that I don't think we should be moving the filter bar around, I think that should remain pinned. 

I like Sebastian's thoughts in Comment 7; would that make you feel about issue 1 that you mentioned in comment 2?
Filter on CLIMBING SHOES
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Assignee: nobody → gl
Attachment #8763291 - Flags: review?(pbrosset) → review?(bgrinstead)
Attachment #8763291 - Flags: review?(bgrinstead)
Depends on: 1267414
No longer depends on: 1267414
Status: NEW → ASSIGNED
Attached patch 1247729.patch [2.0] (obsolete) — Splinter Review
Attachment #8763291 - Attachment is obsolete: true
Attachment #8766366 - Flags: review?(bgrinstead)
Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

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

Sending review to Julian
Attachment #8766366 - Flags: review?(bgrinstead) → review?(jdescottes)
Notes about the patch:
- Added tabindex to the layout view editors and container
- Added tabindex to the computed view property container
- Needed to scrollIntoView in some of the computed view tests because focus/click doesn't work when it is not in view
Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

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

Looks good overall, thanks! Keeping the review flag to discuss a few open questions.

Some additional comments:
- still found two references to computedview.view : devtools/client/inspector/test/head.js::selectComputedView & devtools/client/inspector/shared/test/browser_styleinspector_refresh_when_active.js
- when we select a node that has no box model (such as a comment node) the layoutView is not updated and retains its previous values. It should be emptied or hidden I suppose. This is not a regression, we already had the issue before, but I feel like this patch makes the layout view much more visible to our users so this should either be addressed here or in a follow-up.
- comment #7 mentioned adding a way to toggle the layout view, I assume we are moving this to a follow up?
- can you ask a UI review from Helen?

::: devtools/client/inspector/inspector.xul
@@ -62,5 @@
>               label="&fontInspectorTitle;"
>               crop="end"
>               hidden="true"/>
> -        <tab id="sidebar-tab-layoutview"
> -             label="&layoutViewTitle;"

layoutViewTitle is not used anywhere else and can be removed from devtools/client/locales/en-US/layoutview.dtd

@@ +105,5 @@
>                        label="&browserStylesLabel;"/>
>            </html:div>
>  
> +          <html:div id="computedview-container">
> +            <html:div id="layout-wrapper" tabindex="0">

I think you can get rid of the layout-wrapper, it was only used to fill the height of the panel when the layout view had a dedicated panel.

::: devtools/client/inspector/layout/layout.js
@@ +554,3 @@
>      if (isActive) {
>        this.trackReflows();
> +      this.update();

Not sure about calling update() here. Looking at onNewNode, we are calling setActive() and then update(), so it's weird to have setActive() use update() as well.

In the init method, we are still listening to "layoutview-selected" which will not be fired. I think if you listen to "computedview-selected" instead, we can remove the call to this.update() here.

@@ +560,5 @@
>    },
>  
>    /**
>     * Compute the dimensions of the node and update the values in
> +   * the linspector.xul document.

nit: s/linspector/inspector

::: devtools/client/shared/test/browser_telemetry_sidebar.js
@@ +27,5 @@
>    info("Testing sidebar");
>  
>    let inspector = toolbox.getCurrentPanel();
>    let sidebarTools = ["ruleview", "computedview", "fontinspector",
> +                      "animationinspector"];

Should we also remove the telemetry entries for the layoutview in telemetry.js and Histograms.json ?

::: devtools/client/themes/layout.css
@@ +17,5 @@
>    /* "Contain" the absolutely positioned #layout-main element */
>    position: relative;
>  }
>  
> +@media (max-height: 250px) {

Now that the layout view has to share its panel with the computed view, I feel like the compact version is a better choice most of the time. What about increasing the max-height here ?

When the toolbox is docked to the side, this media query becomes irrelevant. Maybe the compact version should always be used when docked to the side. Also applicable when the toolbox width is lower than 700px.
Attachment #8766366 - Flags: review?(jdescottes) → feedback+
Adding devdoc-needed. MDN pages such as https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour#Box_Model_view will have to be updated to reflect the new UI.
Keywords: dev-doc-needed
Comment on attachment 8766366 [details] [diff] [review]
1247729.patch [2.0]

Took a gander—

I think this will land more easily if we add the arrow for the entire Box model area so it can be collapsed: http://cl.ly/3d44471S2H3K

Let's add a "Box Model" label at the top with the collapsible arrow next to it. (This is the direction the designs are moving anyway: http://cl.ly/1n280L3W0r3o)

It seems like the width declaration for #propertyContainer is wrong. Seems like the horizontal scroll is getting chopped off at an odd point in the container. You can still scroll all the way to the right and read all the properties so it's not major, but it seems like it's been introduced by your patch when I compare to release Nightly.

Lastly, the spacing around the top and bottom of the box model area feel a little large. I think that if you cut the white space between the width/height/static declaration and the top, the width/height/static declaration and the box model, and the box model to the next section each respectively in half that area won't feel so large.
Attached patch 1247729.patch [3.0] (obsolete) — Splinter Review
@jdescottes, this should fix most of your feedback. I am gonna defer the layout view size until I have more of a formal decision/spec from an ui-review with Helen.
Attachment #8766366 - Attachment is obsolete: true
Attachment #8768885 - Flags: ui-review?(hholmes)
Attachment #8768885 - Flags: review?(jdescottes)
Attached patch 1247729.patch [4.0] (obsolete) — Splinter Review
Attachment #8768885 - Attachment is obsolete: true
Attachment #8768885 - Flags: ui-review?(hholmes)
Attachment #8768885 - Flags: review?(jdescottes)
Attachment #8769026 - Flags: ui-review?(hholmes)
Attachment #8769026 - Flags: review?(jdescottes)
Comment on attachment 8769026 [details] [diff] [review]
1247729.patch [4.0]

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

Looks good! Let's do a last round of review after getting the ui-review from Helen and addressing my comments below.

FWIW, I think the toggling is a great improvement compared to the previous version!

::: devtools/client/inspector/layout/layout.js
@@ +189,5 @@
>    this.inspector = inspector;
> +  this.doc = document;
> +  this.wrapper = this.doc.getElementById("layout-wrapper");
> +  this.container = this.doc.getElementById("layout-container");
> +  this.expander = this.doc.getElementById("layout-expander");

make sure wrapper, container and expander are set to null in destroy().

@@ +210,5 @@
>      this.onSidebarSelect = this.onSidebarSelect.bind(this);
>      this.inspector.sidebar.on("select", this.onSidebarSelect);
>  
> +    this.onToggleExpander = this.onToggleExpander.bind(this);
> +    this.expander.addEventListener("click", this.onToggleExpander);

remove event listener in destroy()

@@ +456,5 @@
>    },
>  
>    onSidebarSelect: function (e, sidebar) {
> +    this.setActive(sidebar === "computedview");
> +    return this.update();

Is this.update() really needed ? When we select the computedview, onNewNode is already called and should trigger the update, right?

@@ +483,5 @@
> +        !this.inspector.selection.isElementNode()) {
> +      this.wrapper.hidden = true;
> +      return promise.resolve(undefined);
> +    }
> +

suggestion: maybe move this logic to update(), since we already handle this.wrapper.hidden = false there ? 

IMO:
- if view is hidden -> do not update
- if view is visible but node is invalid -> hide wrapper and do not update
- if view is visible and node is valid -> show wrapper and update

Feel free to keep as is if you prefer!

::: devtools/client/themes/layout.css
@@ +16,5 @@
> +}
> +
> +@media (max-height: 250px) {
> +  #layout-wrapper {
> +    height: 190px;

Collapsing is not working when this media query applies.

If you resize the toolbox to have height < 250px and collapse the box-model view, this rule still applies and the container does not look collapsed.
Attachment #8769026 - Flags: review?(jdescottes) → feedback+
Comment on attachment 8769026 [details] [diff] [review]
1247729.patch [4.0]

Overall looking pretty good! A few small comments:

- Currently there's a thin line around the Box Model area: https://cl.ly/0A1k0u3j3Q2j I think this can be removed for now. Eventually we'll have it but it will have additional functionality attached to it, so until that's implemented I think we only need the small gray dotted line separating the box model from the computed values.
- I notice that the width/height and position declarations are gone. Was that intentional? I think it would be best to leave them in if it was—future designs will display those values as did the past implementation, so I think the transition will be smoother if we leave them in.
Attachment #8769026 - Flags: ui-review?(hholmes) → feedback+
Attached patch 1247729.patch [5.0] (obsolete) — Splinter Review
Attachment #8769026 - Attachment is obsolete: true
Attachment #8769659 - Flags: ui-review?(hholmes)
Attachment #8769659 - Flags: review?(jdescottes)
Attached patch 1247729.patch [6.0] (obsolete) — Splinter Review
Attachment #8769659 - Attachment is obsolete: true
Attachment #8769659 - Flags: ui-review?(hholmes)
Attachment #8769659 - Flags: review?(jdescottes)
Attachment #8769756 - Flags: ui-review?(hholmes)
Attachment #8769756 - Flags: review?(jdescottes)
Attached patch 1247729.patch [7.0] (obsolete) — Splinter Review
Attachment #8769756 - Attachment is obsolete: true
Attachment #8769756 - Flags: ui-review?(hholmes)
Attachment #8769756 - Flags: review?(jdescottes)
Attachment #8769759 - Flags: ui-review?(hholmes)
Attachment #8769759 - Flags: review?(jdescottes)
Comment on attachment 8769759 [details] [diff] [review]
1247729.patch [7.0]

Looking good! One small last thing:

Can we align the box-model arrow with the other arrows so everything is vertically aligned? https://cl.ly/043n1t330615
Attachment #8769759 - Flags: ui-review?(hholmes)
Attached patch 1247729.patch [8.0] (obsolete) — Splinter Review
Attachment #8769759 - Attachment is obsolete: true
Attachment #8769759 - Flags: review?(jdescottes)
Attachment #8769979 - Flags: review?(jdescottes)
Summary: Let's move the box model panel into the computed styles panel. → Move the box model panel into the computed styles panel
Comment on attachment 8769979 [details] [diff] [review]
1247729.patch [8.0]

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

Implementation in layout.js looks good now, just have two more layout/ux issues I spotted while testing (sorry for not seeing those before :/)

First, the #layout-geometry-editor button used to be next to the position label. Now it is displayed next to the
"Box Model" header. Just want to make sure this is intentional?

Second, see my comment in computed.css

::: devtools/client/themes/computed.css
@@ +31,3 @@
>  #propertyContainer {
>    -moz-user-select: text;
>    overflow: auto;

There is a scrollbar issue when a property value forces the property container to overflow. For instance a long url for a background image.

e.g: about:home on a local build, and inspect the #brandLogo element. 

You should see a horizontal scrollbar (on #computedview-container). But if you scroll to the bottom with the vertical scrollbar, there is a second horizontal scrollbar (on #property-container). 

FWIW, the first horizontal scrollbar is caused by the margin-right label "auto" which is rotated and has a line-height of 132px. This already occurs in the standalone layout-view, so this is not a regression by itself, just more annoying now that the layout-view is embedded in the computed view.

Even without considering the first buggy horizontal scrollbar, the user should not have to scroll down to the bottom of the list in order to be able to scroll horizontally.

I think we should remove the overflow auto here, so that the overflow is entirely driven by #computedview-container. It means that the box-model can be scrolled out, but I think this is an acceptable compromise (unless you have a better solution of course!)
Attachment #8769979 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #32)
> First, the #layout-geometry-editor button used to be next to the position
> label. Now it is displayed next to the
> "Box Model" header. Just want to make sure this is intentional?

@Helen, Need info, halp!
Flags: needinfo?(hholmes)
Duplicate of this bug: 1285945
(In reply to Julian Descottes [:jdescottes] from comment #32)
> Comment on attachment 8769979 [details] [diff] [review]
> 
> First, the #layout-geometry-editor button used to be next to the position
> label. Now it is displayed next to the
> "Box Model" header. Just want to make sure this is intentional?
> 

Just as a reference, you can see the edit position "crosshair" icon displayed next to Box Model in the screenshot attached here.
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #33)
> (In reply to Julian Descottes [:jdescottes] from comment #32)
> > First, the #layout-geometry-editor button used to be next to the position
> > label. Now it is displayed next to the
> > "Box Model" header. Just want to make sure this is intentional?
> 
> @Helen, Need info, halp!

I have it positioned there in my designs, so it was intentional from me. Julian, other than noticing that it had changed, is there any cause for concern putting it there that worries you? If so, we can move it back.
Flags: needinfo?(hholmes) → needinfo?(jdescottes)
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #36)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #33)
> > (In reply to Julian Descottes [:jdescottes] from comment #32)
> > > First, the #layout-geometry-editor button used to be next to the position
> > > label. Now it is displayed next to the
> > > "Box Model" header. Just want to make sure this is intentional?
> > 
> > @Helen, Need info, halp!
> 
> I have it positioned there in my designs, so it was intentional from me.
> Julian, other than noticing that it had changed, is there any cause for
> concern putting it there that worries you? If so, we can move it back.

Hadn't noticed it in the mockups that's all, I'm fine with the new position. Since it's hidden for most elements (which is another problem) I thought we might have missed it.
Flags: needinfo?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #37)
> Hadn't noticed it in the mockups that's all, I'm fine with the new position.
> Since it's hidden for most elements (which is another problem) I thought we
> might have missed it.

Ahhh, okay, cool! I attached some designs for some enhancements to the box model in bug 1150496 and it seems like Gabe has been working on them simultaneously, which is why a few things have popped up in this migration bug that perhaps weren't expected.

Sounds like you don't have to worry about this point, Gabe.
Attachment #8769979 - Attachment is obsolete: true
Attachment #8770416 - Flags: review?(jdescottes)
Comment on attachment 8770416 [details] [diff] [review]
1247729.patch [9.0]

@yzen: I added some tabindex/tabbable focus to the box model around the individual margin, padding and border editors. Can you comment if this is desirable?
Attachment #8770416 - Flags: a11y-review?(yzenevich)
Comment on attachment 8770416 [details] [diff] [review]
1247729.patch [9.0]

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

Thanks for addressing all my comments!
Long urls now have an ellipsis which is not ideal, but users can toggle the property and see the full image-url so I think this is ok.
Attachment #8770416 - Flags: review?(jdescottes) → review+
(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #40)
> Comment on attachment 8770416 [details] [diff] [review]
> 1247729.patch [9.0]
> 
> @yzen: I added some tabindex/tabbable focus to the box model around the
> individual margin, padding and border editors. Can you comment if this is
> desirable?

Hi Gabriel, we actually have this bug for box model a11y - bug 1243045. nancy is currently working on it as we had devised a keyboard accessibility plan for it. Would it be alright to land keyboard and other a11y improvements as part of that patch?
Flags: needinfo?(gl)
Blocks: 1243045
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a097b61f5810
Move the box model panel into the computed styles panel r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/a097b61f58102776941bd3fa0023092ac3f27a3f
Bug 1247729 - Move the box model panel into the computed styles panel r=jdescottes
(In reply to Yura Zenevich [:yzen] from comment #42)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #40)
> > Comment on attachment 8770416 [details] [diff] [review]
> > 1247729.patch [9.0]
> > 
> > @yzen: I added some tabindex/tabbable focus to the box model around the
> > individual margin, padding and border editors. Can you comment if this is
> > desirable?
> 
> Hi Gabriel, we actually have this bug for box model a11y - bug 1243045.
> nancy is currently working on it as we had devised a keyboard accessibility
> plan for it. Would it be alright to land keyboard and other a11y
> improvements as part of that patch?

Yes, this would be fine. I have removed the tabindex I have originally added.
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Attachment #8770416 - Flags: a11y-review?(yzenevich)
https://hg.mozilla.org/integration/fx-team/rev/48d51227912b8ec3600758d59a995521e2fbea1c
Bug 1247729 - Move the box model panel into the computed styles panel r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/48d51227912b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1288070
Depends on: 1288205
Depends on: 1288238
Depends on: 1288341
Depends on: 1291673
Duplicate of this bug: 1249013
Verified the new feature on latest Aurora 50.0a2

Build ID 	20160824004001
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
[bugday-20160824]
Status: RESOLVED → VERIFIED
I've updated the pages at:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_the_box_model
and
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/UI_Tour#Computed_view

to describe the new arrangement. I'm going to mark this one dev-doc-complete, but please let me know if you think anything else is needed.
And I've added a note about this change to https://developer.mozilla.org/en-US/Firefox/Releases/50.

Sebastian
Depends on: 1327247
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.