boxmodel-offset-parent overlaps with boxmodel-position

VERIFIED FIXED in Firefox 55

Status

P3
normal
VERIFIED FIXED
2 years ago
6 months ago

People

(Reporter: magicp.jp, Assigned: pbro)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8856761 [details]
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
Blocks: 1150496, 1347964
Priority: -- → P3
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
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.
Attachment #8860994 - Flags: feedback?(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 hidden (mozreview-request)
Attachment #8860994 - Flags: feedback?(gl) → feedback+

Comment 5

2 years ago
mozreview-review
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.
Attachment #8861065 - Flags: review?(gl) → review+
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0cb6f9f404dc
Polish the properties list and remove the position info at the top; r=gl

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0cb6f9f404dc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee: nobody → pbrosset
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?
Flags: qe-verify+
Flags: needinfo?(pbrosset)
Flags: needinfo?(gl)
(Assignee)

Comment 12

2 years ago
(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.
Flags: needinfo?(pbrosset)
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
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.