1355267 - boxmodel-offset-parent overlaps with boxmodel-position

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[magicp]

Attachment: boxmodel-offset-parent.png

[Steps to reproduce]
1. Start latest Nightly
2. Go to about:home
3. Open Inspector > Computed
4. Select #searchIcon in markup

[Actual Results]
boxmodel-offset-parent overlaps with boxmodel-position.

[Expected Results]
Eliminate overlapping

Comment 1

[Patrick Brosset <:pbro>]

I can think of 2 possible ways to fix this:

a - either we move the "position:absolute" and offset parent labels to their own row, above the box-model diagram, so this overlap does not happen
b - or we improve the ElementNode Rep so it has another mode where it's even shorter. Right now we use the "TINY" mode which makes it look like tag#id.class but maybe we should also have a maxLength option for this mode that starts removing classes or using ellipsis to make sure the preview never exceeds a certain length.

I tend to prefer option a right now, but it doesn't have to be either or. I think both are good ideas to implement.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: box-model-offset-node-location.gif

Here's a proposal for the box-model:
- remove the "position:absolute" legend at the top. This information is already in 2 other places,
- move the offset parent preview from the top right corner, to inside the properties view, next to the absolute value,
- add a max-width on the box-model container so that if the panel is really wide, things don't get too far apart, and the whole thing is easier to read,
- add a column layout to the properties view so that if there is more space, we break the list of properties down in several columns (we're more likely to have a narrow sidebar than a wide one, so these 2 last things are less important, but I believe they help polishing still)

See the gif for a better idea of what this looks like.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Patrick Brosset <:pbro> from comment #2)
> Created attachment 8860994 [details]
> box-model-offset-node-location.gif
> 
> Here's a proposal for the box-model:
> - remove the "position:absolute" legend at the top. This information is
> already in 2 other places,
> - move the offset parent preview from the top right corner, to inside the
> properties view, next to the absolute value,
> - add a max-width on the box-model container so that if the panel is really
> wide, things don't get too far apart, and the whole thing is easier to read,
> - add a column layout to the properties view so that if there is more space,
> we break the list of properties down in several columns (we're more likely
> to have a narrow sidebar than a wide one, so these 2 last things are less
> important, but I believe they help polishing still)
> 
> See the gif for a better idea of what this looks like.

This looks great! Just looking at Helen's design again - I think it would be great if we could add some label like [offset] next to the Node Rep to really indicate what we are displaying and its importance. You can see this in Helen's design with the [ancestor] label.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1355267 - Polish the properties list and remove the position info at the top;

Review commit: https://reviewboard.mozilla.org/r/133052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133052/

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8861065 [details]
Bug 1355267 - Polish the properties list and remove the position info at the top;

https://reviewboard.mozilla.org/r/133052/#review136416

Visually, it doesn't look like the offset reference element and the node rep looks centered with eachother.

::: devtools/client/inspector/boxmodel/components/BoxModelProperties.js:50
(Diff revision 1)
> +   * Various properties can display a reference element. E.g. position displays an offset
> +   * parent if its value is other than fixed or static. Or z-index displays a stacking
> +   * context, etc.
> +   * This returns the right element if there needs to be one, and one was passed in the
> +   * props.
> +   * @return {Object} An object with 2 properties:

Add a new line before @return to separate the JSDoc and @params/return

::: devtools/client/inspector/boxmodel/components/BoxModelProperties.js:54
(Diff revision 1)
> +   * props.
> +   * @return {Object} An object with 2 properties:
> +   * - referenceElement {NodeFront}
> +   * - referenceElementType {String}
> +   */
> +  getReferenceElement(propertyName) {

Move this after getInitialState()

::: devtools/client/inspector/boxmodel/components/ComputedProperty.js:40
(Diff revision 1)
> +   *
> +   * @param  {NodeFront} nodeFront
> +   *         The NodeFront for which we want to create a grip-like object.
> +   * @return {Object} a grip-like object that can be used with Reps.
> +   */
> +  translateNodeFrontToGrip(nodeFront) {

Move this before onFocus().

::: devtools/client/inspector/boxmodel/components/ComputedProperty.js:65
(Diff revision 1)
> +        isConnected: true,
> +      }
> +    };
> +  },
> +
> +  referenceElementPreview() {

let's call this renderReferenceElementPreview

::: devtools/client/themes/boxmodel.css:12
(Diff revision 1)
>   */
>  
>  .boxmodel-container {
>    overflow: auto;
> -  padding-bottom: 4px;
> +  max-width: 600px;
> +  margin: 4px auto;

We can't have margin-top/bottom in this case because the focusring would not cover the entire boxmodel-container.

Comment 6

[Patrick Brosset <:pbro>]

Thank you Gabriel. I have addressed your comments and will make sure this passes tests.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)
> Visually, it doesn't look like the offset reference element and the node rep
> looks centered with eachother.
Can you perhaps attach a screenshot that illustrates this problem please? I don't really understand what you mean and I couldn't see an alignment problem locally.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8861065 [details]
Bug 1355267 - Polish the properties list and remove the position info at the top;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133052/diff/1-2/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8861065 [details]
Bug 1355267 - Polish the properties list and remove the position info at the top;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133052/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cb6f9f404dc
Polish the properties list and remove the position info at the top; r=gl

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0cb6f9f404dc

Comment 11

[Julian Descottes [:jdescottes]]

This seems to me like a qe+ bug.

STRs for verification should be in the summary. 
Patrick: I see there's been some discussion on the bug, is there anything else that should be verified here?

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #11)
> This seems to me like a qe+ bug.
> 
> STRs for verification should be in the summary. 
> Patrick: I see there's been some discussion on the bug, is there anything
> else that should be verified here?
Thanks Julian. Yes, things have changed.
The offset parent now isn't displayed at the top anymore. It only appears inside the list of properties below the box-model diagram, when an item is positioned.

Comment 13

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue with an old Nightly build 2017-04-10.

This is verified fixed on 55.0b1 (20170613054006) across platforms:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS