Proper stacking of positive and negative numbers

ASSIGNED
Assigned to

Status

()

Firefox
Developer Tools: Inspector
P2
normal
ASSIGNED
6 months ago
5 months ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

6 months ago
Created attachment 8904363 [details]
negative-stack-issue.png

Stacking negative line numbers looks off in some cases. Please see attachment.
(Assignee)

Updated

5 months ago
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
status-firefox57: --- → fix-optional

Comment 2

5 months ago
mozreview-review
Comment on attachment 8906338 [details]
Bug 1396673 - Negative line numbers are stacking incorrectly.

https://reviewboard.mozilla.org/r/178070/#review186992

::: devtools/server/actors/highlighters/css-grid.js:1250
(Diff revision 1)
>          const { breadth }  = gridLine;
>  
>          if (breadth === 0) {
>            stackedLines.push(negativeLineNumber);
>  
> -          if (stackedLines.length > 0) {
> +          if (stackedLines.length > 0 && stackedIndex === 0) {

I notice now that this approach basically considers just one line per dimension where the number are stacked, but I believe that this is not necessary the case.

See for example this grid, provided by Jen:

https://s.codepen.io/jensimmons/debug/pWvNXa

(test also with different zoom level and viewport size).
There are empty boxes, and also misplaced (because they're considered the "back" of the stack).

That of course, is applying on both positive and negative numbers. We should probably rename this bug something like "stack properly positive and negative numbers", and it should have an higher priority too.

::: devtools/server/actors/highlighters/css-grid.js:1648
(Diff revision 1)
>  
>      [x, y] = apply(this.currentMatrix, [x, y]);
>  
>      if (stackedLineIndex) {
> -      // Offset the stacked line number by half of the box's width/height
> -      const xOffset = boxWidth / 4;
> +      // Offset the stacked line number by a quarter of the box's width/height
> +      const boxOffset = boxHeight / 4;

You should keep both `boxWidth` and `boxHeight`, and therefore `xOffset` and `yOffset`, since one of them could be different from the other. They're equals *only* if the content is not bigger than the minium size.
Attachment #8906338 - Flags: review?(zer0) → review-
(Assignee)

Comment 3

5 months ago
Created attachment 8911638 [details]
Screenshot from 2017-09-24 20-46-50.png

Drawing the number for every stacked line reveals some issues with how negative line numbers are displayed. Notice how "the lowest number of the stack" is brought to the front, however in Jen's example ends up incorrectly showing the sequence of negative line numbers instead.
(Assignee)

Updated

5 months ago
Summary: Negative line numbers are stacking incorrectly → Proper stacking of positive and negative numbers

Updated

5 months ago
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.