Closed Bug 1383327 Opened 7 years ago Closed 7 years ago

Display the grid line number box shape as a directional pointer

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: micah, Assigned: micah)

References

(Blocks 1 open bug)

Details

(Keywords: DevAdvocacy, Whiteboard: [devRel:P1])

Attachments

(5 files)

Currently the grid line number box is a simple rectangle with rounded corners. We should have it so the box will have a pointer shape that indicates its direction (rows or columns) and point. 

See design here: https://github.com/ZER0/grid-negative-numbers/wiki#how-we-could-handle-those-issues-with-negative-numbers
Priority: -- → P3
I *really* like these arrow-boxes!!! I think this is an amazing solution. It's very clear what these mean. 

I grabbed a couple stills from these gifs, because:
A) Almost 0% of developers will be editing a grid that is literally animating. It's a cool way to understand the engineering, but it's not necessarily the best way to see if the intended design works for the intended users.
B) Personally I have a really hard time dealing with this kind of motion like that. I can't really look at repeating animated gifs. Which is just a thing for me (and anyone with that kind of motion sensitivity. But hey — since I'm redoing the images, I might as well share them. 

Here they are: 
https://monosnap.com/file/KklUVcwffQA4kVXLbxGOdFkBKYyCjy.png   << The 80%+ usecase
https://monosnap.com/file/dEPTkARd4RSjc7UDlsTmMBNe1sEpKM.png

Can you also do drawings of how these arrow-boxes will work when we label a line that has more than one line number? That's an important part of this puzzle. What am I talking about? This: https://bugzilla.mozilla.org/show_bug.cgi?id=1380544 (Let me know if you have questions. I'm happy to jump on a video call to explain Grid!)
(In reply to Jen Simmons [:jensimmons] from comment #3)
> I *really* like these arrow-boxes!!! I think this is an amazing solution.
> It's very clear what these mean. 
> 
> I grabbed a couple stills from these gifs, because:
> A) Almost 0% of developers will be editing a grid that is literally
> animating. It's a cool way to understand the engineering, but it's not
> necessarily the best way to see if the intended design works for the
> intended users.
> B) Personally I have a really hard time dealing with this kind of motion
> like that. I can't really look at repeating animated gifs. Which is just a
> thing for me (and anyone with that kind of motion sensitivity. But hey —
> since I'm redoing the images, I might as well share them. 
> 
> Here they are: 
> https://monosnap.com/file/KklUVcwffQA4kVXLbxGOdFkBKYyCjy.png   << The 80%+
> usecase
> https://monosnap.com/file/dEPTkARd4RSjc7UDlsTmMBNe1sEpKM.png
> 
> Can you also do drawings of how these arrow-boxes will work when we label a
> line that has more than one line number? That's an important part of this
> puzzle. What am I talking about? This:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1380544 (Let me know if you
> have questions. I'm happy to jump on a video call to explain Grid!)

In the spirit of Matteo's designs for the arrow-boxes, I imagine overlapping lines would look like this: 

https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/OverlappingPosLineNumbers.png

As you can see the the columns 3..5 are collapsed on top of each other, while the rows 2..5 are collapsed on top of each other as well. 

When we throw negative line numbers into the mix, I imagine it would look like this as well:

https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/OverlappingNegLineNumbers.png

I also put together some existing code from my other tasks to get a live, but simple example that would help demonstrate this:
https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/LiveExampleForOverlapping.png

Of course, please let us know if there is something off in any of the above images. But hopefully it helps you get an idea of what the arrow-box approach might entail.

Also thank you for capturing those stills! For demoing purposes, I'll make sure to upload stills for any future bugs I may work along with gifs that may provide value.
Attached image arrow boxes
(In reply to Micah Tigley [:micah] from comment #2)
> Still image for reference:
> https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/
> demos/NegativeLineNumbers/DirectionalArrows.png

Thanks for working on this, Micah! There are few adjustment I think we should make from UI point of view:

1. I think we could get rid of the tipRound, it's a bit weird that the point is different, the shapes should be consistent; so just having always the sharp point I think it's good.

2. We should have at least a box's minimum width for single digit scenario: the box for 1 seems 1px smaller compare to the rest; I believe we currently use just the width of the text.

3. The numbers doesn't seems well aligned. The ones correct seems the positive cols, but if you look at the rows or negative col, you'll see that the number are a bit misplaced inside the boxes (e.g. the numbers are a bit too close to the bottom's box compare to the top)

4. The shapes don't looks symmetric: this is much evident especially on the negative numbers' boxes (the right side is longer than the left side, before it become an "arrow")



About the code, instead, I think we've too many functions to draw the shapes, and we also risk that one shape could looks different from another (as, for instance, the top boxes compares to the bottom ones). I think it would be better have just one function that creates the shape, and then rotate it based on the alignment.

The attachment shows an example of what I mean (the red dot is just to show where the actual x/y are pointing, so the origin point is shift, plus in this way we can easily  pass a "margin" distance from the pointer for the box)

To obtain that, you can use Path2D instead of drawing directly to the canvas (see: https://developer.mozilla.org/en-US/docs/Web/API/Path2D) the tricky part, you need a SVGMatrix unfortunately to rotate it (it doesn't accept yet a DOMMatrix), and you can create only from DOM. However, we already create a canvas element for the gap, so I don't think it's a big issue.

If you want, I also have a code to draw this kind of shapes that you could adapt to the highlighter.
Attachment #8889725 - Flags: review?(zer0)
I removed my self as reviewer until we discuss about the last comments I made (that wasn't strictly for code review on mozreview); feel free to add me again for the next round!
One last thing: I would try to play implementing the "bubble type" too and see how it goes, since if the pointer is basically the same size, we might avoid visual differences like the one we have in the left bottom corner (positive row 7, negative column -15; https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/DirectionalArrows.png).

But I'm not sure, we have to play with that and see what's looking better in action.
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #5)
> Created attachment 8890819 [details]
> arrow boxes
> 
> (In reply to Micah Tigley [:micah] from comment #2)
> > Still image for reference:
> > https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/
> > demos/NegativeLineNumbers/DirectionalArrows.png
> 
> Thanks for working on this, Micah! There are few adjustment I think we
> should make from UI point of view:
> 
> 1. I think we could get rid of the tipRound, it's a bit weird that the point
> is different, the shapes should be consistent; so just having always the
> sharp point I think it's good.
> 
> 2. We should have at least a box's minimum width for single digit scenario:
> the box for 1 seems 1px smaller compare to the rest; I believe we currently
> use just the width of the text.
> 
> 3. The numbers doesn't seems well aligned. The ones correct seems the
> positive cols, but if you look at the rows or negative col, you'll see that
> the number are a bit misplaced inside the boxes (e.g. the numbers are a bit
> too close to the bottom's box compare to the top)
> 
> 4. The shapes don't looks symmetric: this is much evident especially on the
> negative numbers' boxes (the right side is longer than the left side, before
> it become an "arrow")
> 
> 
> 
> About the code, instead, I think we've too many functions to draw the
> shapes, and we also risk that one shape could looks different from another
> (as, for instance, the top boxes compares to the bottom ones). I think it
> would be better have just one function that creates the shape, and then
> rotate it based on the alignment.
> 
> The attachment shows an example of what I mean (the red dot is just to show
> where the actual x/y are pointing, so the origin point is shift, plus in
> this way we can easily  pass a "margin" distance from the pointer for the
> box)
> 
> To obtain that, you can use Path2D instead of drawing directly to the canvas
> (see: https://developer.mozilla.org/en-US/docs/Web/API/Path2D) the tricky
> part, you need a SVGMatrix unfortunately to rotate it (it doesn't accept yet
> a DOMMatrix), and you can create only from DOM. However, we already create a
> canvas element for the gap, so I don't think it's a big issue.
> 
> If you want, I also have a code to draw this kind of shapes that you could
> adapt to the highlighter.

Thanks for the feedback! 

1) I'm not opposed to having either sharp or rounded tips for the arrow-box shape, so changing them to pointed is definitely fine with me. The reasoning behind making the tip rounded was because the arrow-boxes in the mock-ups initially gave me that impression (but now that I look at them again, they do look pointy).

2) Yes the code is currently using the width of the number text to determine its width. We can go with the "x" character for the box's minimum width, or "m" since that is what is being used for measuring the box height. I am not sure it makes a noticeable difference.

3) This is due to the text for a negative line number being wider compared to its positive counterpart. We can probably adjust the padding values for the negative line numbers so that they are centered. 

4) Yes I noticed this issue too. But like you said, this is because I draw each arrow-box individually rather than using a single function. So being able refactor everything into a single function would be nice! Also, less code :)

Also, it would be great if you could provide the code for drawing those shapes! It would definitely help make the process faster.
Forgot to add the needinfo? tag for the last comment ^
Flags: needinfo?(zer0)
(In reply to Micah Tigley [:micah] from comment #8)

> 1) I'm not opposed to having either sharp or rounded tips for the arrow-box
> shape, so changing them to pointed is definitely fine with me. The reasoning
> behind making the tip rounded was because the arrow-boxes in the mock-ups
> initially gave me that impression (but now that I look at them again, they
> do look pointy).

I'm sorry, for the mockup I used a thick pen so it's probably lost some details! The comment however was more about the fact that some boxes looks pointy than others.

> 2) Yes the code is currently using the width of the number text to determine
> its width. We can go with the "x" character for the box's minimum width, or
> "m" since that is what is being used for measuring the box height. I am not
> sure it makes a noticeable difference.

I think either it's fine, just having the same minimum size that works for all single digit.

> 3) This is due to the text for a negative line number being wider compared
> to its positive counterpart. We can probably adjust the padding values for
> the negative line numbers so that they are centered.

Notice that also the positive rows as the negative ones, they're a bit too close to the bottom.

> 4) Yes I noticed this issue too. But like you said, this is because I draw
> each arrow-box individually rather than using a single function. So being
> able refactor everything into a single function would be nice! Also, less
> code :)
 
> Also, it would be great if you could provide the code for drawing those
> shapes! It would definitely help make the process faster.

Here the code, hope it helps! Feel free to ping me on IRC or Slack (I don't use this a lot, but I'm trying :) ):

https://codepen.io/zer0/pen/yoBNeO

As mentioned, the red dot is just for testing purpose; also you will need some adjustment to make it working on the highlighter, but at least the logic is there. Play with both the bubble type and the other one (there are both functions) and see what looks better.
Also, feel free to add constant and change some naming convention (like `m` and the number used for the bubble's arrow); this was just a quick code.
Flags: needinfo?(zer0)
Thank you for the code sample! I have it implemented into two separate patches. You can view the code here:

https://github.com/TigleyM/firefox-devtools-patches/blob/master/WIP/Bug1383327_ArrowedRoundedRect.patch
https://github.com/TigleyM/firefox-devtools-patches/blob/master/WIP/Bug1383327_BubbleRect.patch

I thought I would provide links to images that demonstrate the two shapes in case anyone else wanted to view:

Image for Arrow Rounded Box (Our current approach)
https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/ArrowRoundedRect.png

Image for Bubble Rect 
https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/BubbleRect.png
I recorded some of my thoughts about how these line number boxes should work, especially as a stack, and in relationship to which line numbers we display when a line has more than one number. Please watch and let me know what you think.

https://vimeo.com/227804515/aa2a16ac9f 

Here are a few still images to look at as well. Things to consider without listening to me ramble.
[1]https://monosnap.com/file/qry5vvMCg4DEJrgJLUiibWlSLXlrRl.png
[2]https://monosnap.com/file/qzY3Xo4HIlpEhHdCyyp1no6WxohBoH.png
(although, watch the video so you know what you are looking at)

This summarizes what I think we should do:

[3]https://monosnap.com/file/Iet19RHWdaBANzpE5uYM1KhXHc9EhY.png


Towards the end of the video, I comment on a possible way to put the negative and positive line numbers on the same side. Here's a sketch of that:
https://monosnap.com/file/VA8teroUOVlIgfdIbtiYTDC01D8cDX.png
@gl was just asking me about when a grid is transformed. I think everything about the boxes should transform together, while the numbers stay upright.

Like this: https://monosnap.com/file/2VPqaudo1X367ppgWsLzRMNuBAxf9w.png
Here my thoughts about Jen's video (we'll discuss in details in the upcoming meeting):

1. About which number showing on top, let's take the positive number for example; and assuming we have:

   1, 2, …6, 7 

Basically here we shows after the number "2" the number "6" with a stack, and then the number "7"
That was made because if we do the opposite, as the video suggests:

   1, 2, 3…, 7

We have the problem that if also the 7 stacks, there is no way to tell which is the last, since it's under "3":

   1, 2, 3…

Where, instead, the same scenario would be different with the current approach:

   1, 2, …7

It's boundaries of the stack are clearer.


2. I totally agree that we should change the orientation of the stack depends by which number is shown on the top! It makes more sense having, in the current approach, the underlining boxes going up.


3. The negative numbers was put on the opposite site to avoid having doubling boxes one on top of the other, since it would be taken more space and messy in several cases (e.g. the corners, when the grid is close to the viewport, when the grid is small, like 1em row, etc.) so I would avoid this solution (I think we explain all the scenarios where it could be a problem, and we agreed on that the last meeting I shown my proposal of "tag shape boxes", but Jen wasn't there).


4. We decided to have a “symbolic” box for stack, instead of drawing the actual boxes behind, because it would be a visual mess (https://bugzilla.mozilla.org/show_bug.cgi?id=1380544#c3, specifically the attchment); so currently it draws only 1 additional box under. I agreed (as I mention in the comment linked) that we could have an improvement where we can draw 1 additional boxes for 2 lines stacked, and 2 additional boxes for three or more. I think more than 3 starts to have an impact on the space taken (and in UX usually “three” is what we used for “more”, see also the ellipsis for example).
We could, however, draw a little “plus” after the three, maybe?


5. The boxes needs to be oriented in the same way the numbers are, because they share a deep connection: the size of the boxes is given by the size of the numbers, if we rotate the boxes they will be too big especially for two digit negative numbers, they will change constantly at every transformation. Also, the pointy shape / tag shape was made precisely to keep the tag point on a specific vertex so that it was clear even during the transformation which row/number is.
I don't believe that rotating the boxes will bring any benefit from UX point of view, and it would makes the code much more complex, even without taking in account edge cases. I think the gif Micah produced gives a nice idea (https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/TransformationsForArrows.gif)

6. For the same reason (and point 3) we shouldn’t have the negative number on the same side of positive one, since if one of them is missing, we'll have the boxes looks misplaced (and during transformation it would be more messy, I think we discussed that too in the last meeting about negative numbers).
Attachment #8889725 - Flags: review?(zer0)
Keywords: DevAdvocacy
Whiteboard: [devRel:P1]
Comment on attachment 8889725 [details]
Bug 1383327 - Display the grid line number box shape as a directional pointer.

https://reviewboard.mozilla.org/r/160798/#review173010

Thanks Micah for working on this! Overall it's good, but I found serveral visual issues that I think we should address before land this (attachment will follows).

::: devtools/server/actors/highlighters/css-grid.js:294
(Diff revision 3)
> + * @param  {Number} xOffset
> + *         The distance from the x origin.
> + * @param  {String} alignment
> + *         The alignment of the rectangle in relation to its position to the grid.
> + */
> +function drawBubbleRect(ctx, x, y, width, height, radius, margin, xOffset, alignment) {

Is there any particular reason why you didn't use `Path2D` here?

::: devtools/server/actors/highlighters/css-grid.js:1605
(Diff revision 3)
> +    let textHeight = this.ctx.measureText("m").width * displayPixelRatio;
>  
> -    // The width of the character 'm' approximates the height of the text.
> -    let textHeight = this.ctx.measureText("m").width;
> +    // Depending on the dimension type, we will determine its horizontal width
> +    // using the width of the characters "xx".
> +    if (dimensionType === COLUMNS) {
> +      textHeight = this.ctx.measureText("xx").width;

It seems odd to me that in some cases we don't apply the `displayPixelRatio` since the measurement it's the same and the display pixel ratio doesn't change. I believe that, if this is needed, there is something wrong with how we use this values.

::: devtools/server/actors/highlighters/css-grid.js:1679
(Diff revision 3)
>      this.ctx.strokeStyle = this.color;
>      this.ctx.fillStyle = "white";
>      let radius = 2 * displayPixelRatio;
> -    drawRoundedRect(this.ctx, x, y, boxWidth, boxHeight, radius);
> +    let margin = 2 * displayPixelRatio;
> +    let xOffset = 8 * displayPixelRatio;
> +    let drawBox = drawBubbleRect;

In my example code I used `drawBox` to switch between bubbled rect and regular one; since here we use always the bubbled rect, we can just avoid this and call directly `drawBubbleRect`.
Attachment #8889725 - Flags: review?(zer0) → review-
If you zoom you'll probably see it better: it seems that one size of the box is smaller than the actual size of the arrow.
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #18)
> Created attachment 8896547 [details]
> arrow misplaced in 2dppx
> 
> If you zoom you'll probably see it better: it seems that one size of the box
> is smaller than the actual size of the arrow.

Also, noticed that the numbers' alignment in 2ddpx is different from 1ddpx (they're more close to the corner).
The numbers are not aligned to the box's center, but looks like more on the box's base (before the arrow). The actual screenshot is on the left; at the right I added what I think they should be looks like (despite the dppx, as I said currently the ddpx changed the way the numbers are aligned).
Attached image Negative numbers' stack
I think this issue is not strictly related to this bug, we could therefore fix in a follow up bug. However, the negative number stacks seems have a problem (check the bottom left corner).
Comment on attachment 8889725 [details]
Bug 1383327 - Display the grid line number box shape as a directional pointer.

https://reviewboard.mozilla.org/r/160798/#review173010

> Is there any particular reason why you didn't use `Path2D` here?

I actually ran into some issues using `2DPath`. It's not available as a global constructor so when I tried using it I would get `2DPath is undefined` and the grid canvas wouldn't render at all. Do you know if I need to import it from somewhere? I wasn't able to find instances of `2DPath` being used in the devtools codebase to figure out how to have access to it.

To work around not being able to use `2DPath` I ended up creating a rotation matrix to rotate the origin point and then draw the shape.
(In reply to Matteo Ferretti [:zer0] [:matteo][PTO till  Aug 19th]] from comment #17)
> Comment on attachment 8889725 [details]
> Bug 1383327 - Display the grid line number box shape as a directional
> pointer.
> 
> https://reviewboard.mozilla.org/r/160798/#review173010
> 
> Thanks Micah for working on this! Overall it's good, but I found serveral
> visual issues that I think we should address before land this (attachment
> will follows).
> 
> ::: devtools/server/actors/highlighters/css-grid.js:294
> (Diff revision 3)
> > + * @param  {Number} xOffset
> > + *         The distance from the x origin.
> > + * @param  {String} alignment
> > + *         The alignment of the rectangle in relation to its position to the grid.
> > + */
> > +function drawBubbleRect(ctx, x, y, width, height, radius, margin, xOffset, alignment) {
> 
> Is there any particular reason why you didn't use `Path2D` here?
> 
> ::: devtools/server/actors/highlighters/css-grid.js:1605
> (Diff revision 3)
> > +    let textHeight = this.ctx.measureText("m").width * displayPixelRatio;
> >  
> > -    // The width of the character 'm' approximates the height of the text.
> > -    let textHeight = this.ctx.measureText("m").width;
> > +    // Depending on the dimension type, we will determine its horizontal width
> > +    // using the width of the characters "xx".
> > +    if (dimensionType === COLUMNS) {
> > +      textHeight = this.ctx.measureText("xx").width;
> 
> It seems odd to me that in some cases we don't apply the `displayPixelRatio`
> since the measurement it's the same and the display pixel ratio doesn't
> change. I believe that, if this is needed, there is something wrong with how
> we use this values.
> 
> ::: devtools/server/actors/highlighters/css-grid.js:1679
> (Diff revision 3)
> >      this.ctx.strokeStyle = this.color;
> >      this.ctx.fillStyle = "white";
> >      let radius = 2 * displayPixelRatio;
> > -    drawRoundedRect(this.ctx, x, y, boxWidth, boxHeight, radius);
> > +    let margin = 2 * displayPixelRatio;
> > +    let xOffset = 8 * displayPixelRatio;
> > +    let drawBox = drawBubbleRect;
> 
> In my example code I used `drawBox` to switch between bubbled rect and
> regular one; since here we use always the bubbled rect, we can just avoid
> this and call directly `drawBubbleRect`.

Thanks for the review Matteo! I have fixed the issues you mentioned above as well as some info on why 2DPath wasn't used.
Comment on attachment 8889725 [details]
Bug 1383327 - Display the grid line number box shape as a directional pointer.

https://reviewboard.mozilla.org/r/160798/#review179788

Thanks Micah for working on this, and my apologies for the delay. The numbers are not aligned properly and there is a difference in the box size on 2dppx display compare to 1ddpx, but I believe with the changes I indicates everything should works!
I'm r- only because I want to have another round after you applied those changes to ensure I didn't forget anything.
I promise this time I will review it immediately. :)

The negative number stack still have issues; do we have a follow up bug for that? About the `Path2D` I believe is a WebAPI so it needs a `window` reference. We could obtain it of course, but for the time being it's perfectly fine the approach you did!

::: devtools/server/actors/highlighters/css-grid.js:294
(Diff revision 4)
> + * @param  {Number} xOffset
> + *         The distance from the x origin.
> + * @param  {String} alignment
> + *         The alignment of the rectangle in relation to its position to the grid.
> + */
> +function drawBubbleRect(ctx, x, y, width, height, radius, margin, xOffset, alignment) {

rename `xOffset` in something like `arrowSize` or similar – especially since it's confusing later on when we're calling this function, there is another `xOffset` used for something else.

::: devtools/server/actors/highlighters/css-grid.js:295
(Diff revision 4)
> + *         The distance from the x origin.
> + * @param  {String} alignment
> + *         The alignment of the rectangle in relation to its position to the grid.
> + */
> +function drawBubbleRect(ctx, x, y, width, height, radius, margin, xOffset, alignment) {
> +  width = Math.max(width, 18);

Remove both line 295 and 296; I think it was there from my codepen code, but in this implementation are wrong.

::: devtools/server/actors/highlighters/css-grid.js:1599
(Diff revision 4)
>      let fontSize = (GRID_FONT_SIZE * displayPixelRatio);
>      this.ctx.font = fontSize + "px " + GRID_FONT_FAMILY;
>  
> -    let textWidth = this.ctx.measureText(lineNumber).width;
> -
> -    // The width of the character 'm' approximates the height of the text.
> +    // For a general grid box, the width of the character "x" will be the minimum width.
> +    // The width of the "m" character will be the grid box's minimum height.
> +    let textWidth = this.ctx.measureText("x").width * displayPixelRatio;

Change both `textWidth` and `textHeight` declaration as follow, and modifying the comment accordingly:
```js
let textHeight = this.ctx.measureText("m").width;
let textWidth = Math.max(textHeight, this.ctx.measureText(lineNumber).width);
```

This because we want to have a "box" – so both height and width the same – unless the `lineNumber` as text is greater than the default size. And `"m"` is a better letter for estimate the `height` instead of `"x"`.

::: devtools/server/actors/highlighters/css-grid.js:1602
(Diff revision 4)
> -    let textWidth = this.ctx.measureText(lineNumber).width;
> -
> -    // The width of the character 'm' approximates the height of the text.
> -    let textHeight = this.ctx.measureText("m").width;
> +    // For a general grid box, the width of the character "x" will be the minimum width.
> +    // The width of the "m" character will be the grid box's minimum height.
> +    let textWidth = this.ctx.measureText("x").width * displayPixelRatio;
> +    let textHeight = this.ctx.measureText("x").width * displayPixelRatio;
> +
> +    // Any box containing a two-digit number will instead use the number's text

Remove this block (from line 1602 to 1606).

::: devtools/server/actors/highlighters/css-grid.js:1671
(Diff revision 4)
>      this.ctx.fillStyle = "white";
> +
> +    // See param definitions of drawBubbleRect
>      let radius = 2 * displayPixelRatio;
> -    drawRoundedRect(this.ctx, x, y, boxWidth, boxHeight, radius);
> +    let margin = 2 * displayPixelRatio;
> +    let xOffset = 8 * displayPixelRatio;

As said, it's a bit confusing redeclare in this scope `xOffset`, I would name it differently – also because it would be used for `height` as well, not only `width`. You can call it `arrowSize` or similar.

::: devtools/server/actors/highlighters/css-grid.js:1672
(Diff revision 4)
> +
> +    // See param definitions of drawBubbleRect
>      let radius = 2 * displayPixelRatio;
> -    drawRoundedRect(this.ctx, x, y, boxWidth, boxHeight, radius);
> +    let margin = 2 * displayPixelRatio;
> +    let xOffset = 8 * displayPixelRatio;
> +

Here you can set the minimum box's size:
```js
  let minBoxSize = arrowSize * 2 + padding;
  boxWidth = Math.max(boxWidth, minBoxSize);
  boxHeight = Math.max(boxHeight, minBoxSize);
```

::: devtools/server/actors/highlighters/css-grid.js:1675
(Diff revision 4)
> -    drawRoundedRect(this.ctx, x, y, boxWidth, boxHeight, radius);
> +    let margin = 2 * displayPixelRatio;
> +    let xOffset = 8 * displayPixelRatio;
> +
> +    if (dimensionType === COLUMNS) {
> +      if (lineNumber > 0) {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,

That would become:
```js
drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, arrowSize, "top");
```

I prefer to explict the direction, since it would makes the code clearer (of course you have to rename the `xOffset` below in the other calls too).

::: devtools/server/actors/highlighters/css-grid.js:1679
(Diff revision 4)
> +      if (lineNumber > 0) {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          null);
> +        // After drawing the number box, we need to center the x/y coordinates of the
> +        // number text written it.
> +        x -= xOffset - margin;

Here you have to adjust only `y`, so remove this line.

::: devtools/server/actors/highlighters/css-grid.js:1680
(Diff revision 4)
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          null);
> +        // After drawing the number box, we need to center the x/y coordinates of the
> +        // number text written it.
> +        x -= xOffset - margin;
> +        y -= (2 * xOffset) + textHeight + margin;

Change this line into:
```js
y -= (boxHeight + arrowSize + radius) - boxHeight / 2;
```

::: devtools/server/actors/highlighters/css-grid.js:1684
(Diff revision 4)
> +        x -= xOffset - margin;
> +        y -= (2 * xOffset) + textHeight + margin;
> +      } else {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "bottom");
> +        x -= xOffset;

As for "top", you need only to adjust `y`, so remove this line.

::: devtools/server/actors/highlighters/css-grid.js:1685
(Diff revision 4)
> +        y -= (2 * xOffset) + textHeight + margin;
> +      } else {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "bottom");
> +        x -= xOffset;
> +        y -= (-2.5 * xOffset) + textHeight + margin;

Change this into:
```js
y += (boxHeight + arrowSize + radius) - boxHeight / 2;
```

::: devtools/server/actors/highlighters/css-grid.js:1688
(Diff revision 4)
> +          "bottom");
> +        x -= xOffset;
> +        y -= (-2.5 * xOffset) + textHeight + margin;
> +      }
> +
> +      if (lineNumber < -9 || lineNumber > 9) {

You don't need this block anymore (from 1688 to 1690).

::: devtools/server/actors/highlighters/css-grid.js:1695
(Diff revision 4)
> +      }
> +    } else if (dimensionType === ROWS) {
> +      if (lineNumber > 0) {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "left");
> +        x += (-3.25 * xOffset) + margin;

In that case you need to keep `x`, so change this into:
```js
x -= (boxWidth + arrowSize + radius) - boxWidth / 2;
```

::: devtools/server/actors/highlighters/css-grid.js:1696
(Diff revision 4)
> +    } else if (dimensionType === ROWS) {
> +      if (lineNumber > 0) {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "left");
> +        x += (-3.25 * xOffset) + margin;
> +        y -= textHeight;

Remove `y` and the block below (so basically everything from line 1696 to 1700)

::: devtools/server/actors/highlighters/css-grid.js:1704
(Diff revision 4)
> +          x -= margin;
> +        }
> +      } else {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "right");
> +        x += xOffset + margin;

Here the same, change this into:
```js
x += (boxWidth + arrowSize + radius) - boxWidth / 2;
```

::: devtools/server/actors/highlighters/css-grid.js:1705
(Diff revision 4)
> +        }
> +      } else {
> +        drawBubbleRect(this.ctx, x, y, boxWidth, boxHeight, radius, margin, xOffset,
> +          "right");
> +        x += xOffset + margin;
> +        y -= textHeight;

And remove this line.

::: devtools/server/actors/highlighters/css-grid.js:1709
(Diff revision 4)
> +        x += xOffset + margin;
> +        y -= textHeight;
> +      }
> +    }
>  
>      // Write the line number inside of the rectangle.

So, all the changes I wrote above is to make sure that `x` and `y` are poiting exactly at the center of the box.
Therefore, if you add after this comment:

```js
this.ctx.textAlign = "center";
this.ctx.textBaseline = "middle";
```

The text will be perfectly centered without additional calculation.

::: devtools/server/actors/highlighters/css-grid.js:1712
(Diff revision 4)
> +    }
>  
>      // Write the line number inside of the rectangle.
>      this.ctx.fillStyle = "black";
>      const numberText = stackedLineIndex ? "" : lineNumber;
>      this.ctx.fillText(numberText, x + padding, y + textHeight + padding);

That also means, you don't need the padding here:
```js
this.ctx.fillText(numberText, x, y);
```
Attachment #8889725 - Flags: review?(zer0) → review-
Comment on attachment 8889725 [details]
Bug 1383327 - Display the grid line number box shape as a directional pointer.

https://reviewboard.mozilla.org/r/160798/#review179788

Thank you for the review! I definitely learned a lot working on this. 

I suppose after the final rounds of reviewing this bug, we would still have to take into consideration the existing UX is compromised since the line numbers are now offset from the edge of the grid container rather than centered on it. So some grid layouts whose edges are close to the edge of the viewport are lost. I imagine we would have to wait on https://bugzilla.mozilla.org/show_bug.cgi?id=1382284 to fix this issue. Or are we planning on hiding this behind a pref until sticky line numbers are landed?
Comment on attachment 8889725 [details]
Bug 1383327 - Display the grid line number box shape as a directional pointer.

https://reviewboard.mozilla.org/r/160798/#review180388

Looks good! Thanks!
Attachment #8889725 - Flags: review?(zer0) → review+
(In reply to Micah Tigley [:micah] from comment #26)

> I suppose after the final rounds of reviewing this bug, we would still have
> to take into consideration the existing UX is compromised since the line
> numbers are now offset from the edge of the grid container rather than
> centered on it.

Yes, that would be a follow-up bug. So, if don't have already those two, we should file them:
1. Negative numbers stacking incorrectly
2. Flip the box's number position if there is no enough space

Basically the 2nd one is referring to: https://github.com/ZER0/grid-negative-numbers/wiki#extreme-case-a-grid-that-is-big-as-the-viewport-no-scrolling

> So some grid layouts whose edges are close to the edge of
> the viewport are lost. I imagine we would have to wait on
> https://bugzilla.mozilla.org/show_bug.cgi?id=1382284 to fix this issue.

I wouldn't. Change the position of the box's numbers is much trivial task than make them sticky. I suspect the sticky would take more time to address them in a way that is not a performance killer; so I wouldn't block the flipping of the boxes – since having a grid close to the edge to the viewport is not uncommon.

So my suggestion would be address this issue ASAP as follow up bug.
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ef17365243
Display the grid line number box shape as a directional pointer. r=zer0
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #29)
> (In reply to Micah Tigley [:micah] from comment #26)
> 
> > I suppose after the final rounds of reviewing this bug, we would still have
> > to take into consideration the existing UX is compromised since the line
> > numbers are now offset from the edge of the grid container rather than
> > centered on it.
> 
> Yes, that would be a follow-up bug. So, if don't have already those two, we
> should file them:
> 1. Negative numbers stacking incorrectly
> 2. Flip the box's number position if there is no enough space
> 
> Basically the 2nd one is referring to:
> https://github.com/ZER0/grid-negative-numbers/wiki#extreme-case-a-grid-that-
> is-big-as-the-viewport-no-scrolling
> 
> > So some grid layouts whose edges are close to the edge of
> > the viewport are lost. I imagine we would have to wait on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1382284 to fix this issue.
> 
> I wouldn't. Change the position of the box's numbers is much trivial task
> than make them sticky. I suspect the sticky would take more time to address
> them in a way that is not a performance killer; so I wouldn't block the
> flipping of the boxes – since having a grid close to the edge to the
> viewport is not uncommon.
> 
> So my suggestion would be address this issue ASAP as follow up bug.

Thanks! I filed the respective bugs here:

1. Negative numbers stacking incorrectly - bug 1396673
2. Flip the box's number position if there is no enough space - bug 1396666
https://hg.mozilla.org/mozilla-central/rev/f6ef17365243
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.