Proper stacking of positive and negative numbers

NEW
Unassigned

Status

P2
normal
2 years ago
8 months ago

People

(Reporter: micah, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified

Firefox Tracking Flags

(firefox57 fix-optional)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8904363 [details]
negative-stack-issue.png

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

Updated

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

Comment 2

a year 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-
(Reporter)

Comment 3

a year 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.
(Reporter)

Updated

a year ago
Summary: Negative line numbers are stacking incorrectly → Proper stacking of positive and negative numbers
Priority: -- → P2
Assignee: tigleym → nobody
Status: ASSIGNED → NEW

Updated

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