Closed Bug 1380544 Opened 7 years ago Closed 7 years ago

Overlapping grid line numbers should display as a stack

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: micah, Assigned: micah)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Currently, overlapping grid line numbers render on top of each other. This causes confusion with grid containers whose rows/columns have a breadth of 0. We should display overlapping grid line numbers as a stack to make it clear the row/columns lines are overlapping.
Attachment #8885997 - Flags: review?(gl) → review?(zer0)
Comment on attachment 8885997 [details]
Bug 1380544 - Overlapping grid line numbers should display as a stack.

https://reviewboard.mozilla.org/r/156782/#review164062

Thanks for working on this, Micah!
The approach is good, but as it is now it gets too crowd when there are multiple lines overlapping. The idea is displaying just an indicator that there is a stack of lines, and not draw all the line numbers actually stacked. I'm going to attach a screenshot about what I mean exactly, and what is the issue with the current code.
Attachment #8885997 - Flags: review?(zer0) → review-
Attached image stack mockup
At the left is the stacking with the current patch. At the right what it should looks like.

Drawing actually all the boxes makes the stack a bit too crowd in certain cases, plus doesn't really help to understand how many lines there are anyway. So just an indicator would be cleaner. The collapsed lines are anyway in sequence, so we now how many lines are there just look at the number on top, and compare to adjacent ones.

Some logic about implementation could be:

1. detect the the collapsed lines, and do not draw anything until the last one (in the attachment would be the 33)
2. at that point draw just the 33, with the specific box for stacked lines.

Notice that in point 2 we'll use the same width of 33 box for the underling boxes, that also helps in case lines with different text's width are overlapping (e.g. 9 and 10); we'll have a consistent width.

We could maybe use two boxes if there are just two lines that are overlaps, and three if there are three or more. But this is definitely a "nice to have", not mandatory for land the feature!

Thanks, and feel free to ping me if you have any questions!
Comment on attachment 8890204 [details]
Bug 1380544 - Overlapping grid line numbers should display as a stack.

https://reviewboard.mozilla.org/r/161288/#review166714

Looks great! Just one nit to fix, and it's ready to land. Thanks for your work!

::: devtools/server/actors/highlighters/css-grid.js:1505
(Diff revision 1)
> +    if (stackedLineIndex) {
> +      // Offset the stacked line number by half of the box's width/height
> +      const xOffset = boxWidth / 4;
> +      const yOffset = boxHeight / 4;
> +
> +      x += xOffset * displayPixelRatio;

You don't have to multiply for the `displayPixelRatio` here, since it's already done in `padding`, that is used for both `boxWidth` and `boxHeight`.
Attachment #8890204 - Flags: review?(zer0) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14755efcbf65
Overlapping grid line numbers should display as a stack. r=zer0
https://hg.mozilla.org/mozilla-central/rev/14755efcbf65
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.