Overlapping grid line numbers should display as a stack

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

10 months ago
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.
Comment hidden (mozreview-request)

Updated

9 months ago
Attachment #8885997 - Flags: review?(gl) → review?(zer0)

Comment 2

9 months ago
mozreview-review
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-
Created attachment 8887889 [details]
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 hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
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+

Comment 6

9 months ago
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

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14755efcbf65
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.