Closed Bug 1396666 Opened 7 years ago Closed 6 years ago

Flip the grid line box's number position if there is not enough space along the container edge

Categories

(DevTools :: Inspector, enhancement, P2)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: micah, Assigned: micah)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(3 files, 1 obsolete file)

If there not enough space to align a grid line's number box, then we should flip its position so that it is aligned on the inside of the grid container's edge.
Summary: Flip the grid line box's number position if there is no enough space → Flip the grid line box's number position if there is not enough space along the container edge
Severity: normal → enhancement
Priority: -- → P3
Matteo, ping on this review. Can you please take a look at it? Or let Micah know when you'll be able to (feel free to redirect to someone else if you're swamped).
Flags: needinfo?(zer0)
(In reply to Patrick Brosset <:pbro> from comment #3)
> Matteo, ping on this review. Can you please take a look at it? Or let Micah
> know when you'll be able to (feel free to redirect to someone else if you're
> swamped).

Before going in PTO I was asking Jen to provide to me some grid examples since I believed the current implementation could have some problem; she gave to me some code last Thursday that I was able to test only yesterday, and they confirm my suspects about displaying the numbers. That was a blocker for me before applying the r+, especially for bug 1396673, but I wanted also to wait the tests before proceed with this one as well.
Flags: needinfo?(zer0)
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review186570

The current implementation doesn't work as expected, I mentioned some issue to fix but I believe the approach needs to be rethink; I will attach some screenshot to shows what's happening.

The major issues are two:

1. You take in consideration the viewport size, but not the scrolling: it means, for instance, that if the viewport doesn't have enough space to display the negative numbers, they're flipped, but if I scroll and THEN there is enough space, the numbers are still flipped. The same if the grid is bigger than the viewport, they're positioned flipped already even if you don't see them, since they're over the viewport. That's because the drawing are not happening on scrolling, for performance reasons (as you know because the sticky numbers patch). 
So the possible solution are two: one, you can update the numbers during the scrolling. However, to keep the performance, you should do the calculation if they need to be flipped on `scrollUpdate`, and if they need, then you call `update`, similar as we do for the canvas' position. That's definitely the solution requires more code.
The other solution is just simpler, and I think for the time being we could go for it and maybe file a follow up bug: don't flip the numbers based on the viewport's size; but based on the document's size. That's would keep more or less the same approach you have now.

2. The flipping are not working on a display that is bigger than 1 dppx; it means they're flipped earlier than expected on my Mac.
I notice that this is something you miss sometimes, so my guess is that you don't have a retina display or similar where you can test your patch. Luckily, in Firefox there is a workaround: you can navigate to "about:config" and set `layout.css.devPixelsPerPx` equals to the dppx you want to test. By default is `-1`, it means the same dppx of the hardware. If you want to test with a retina display, you can set `2` for example. You will see the whole interface of Firefox changes.

::: devtools/server/actors/highlighters/css-grid.js:1668
(Diff revision 2)
>      this.ctx.fillStyle = "white";
>  
>      // See param definitions of drawBubbleRect
>      let radius = 2 * displayPixelRatio;
>      let margin = 2 * displayPixelRatio;
> -    let arrowSize = 8 * displayPixelRatio;
> +    let arrowSize = 7 * displayPixelRatio;

Why you decrease by `1` the arrow's size?

::: devtools/server/actors/highlighters/css-grid.js:1669
(Diff revision 2)
>  
>      // See param definitions of drawBubbleRect
>      let radius = 2 * displayPixelRatio;
>      let margin = 2 * displayPixelRatio;
> -    let arrowSize = 8 * displayPixelRatio;
> +    let arrowSize = 7 * displayPixelRatio;
> +    let minOffsetFromEdge = 25 * displayPixelRatio;

Where this `25` comes from?

::: devtools/server/actors/highlighters/css-grid.js:1670
(Diff revision 2)
>      // See param definitions of drawBubbleRect
>      let radius = 2 * displayPixelRatio;
>      let margin = 2 * displayPixelRatio;
> -    let arrowSize = 8 * displayPixelRatio;
> +    let arrowSize = 7 * displayPixelRatio;
> +    let minOffsetFromEdge = 25 * displayPixelRatio;
> +    const scrollYPos = this.win.scrollY / currentZoom;

You should use `this._scroll.y` instead, and I'm not sure you should divide by the zoom.

::: devtools/server/actors/highlighters/css-grid.js:1682
(Diff revision 2)
> +    let textCenterPos;
> +
>      if (dimensionType === COLUMNS) {
>        if (lineNumber > 0) {
> +        boxAlignment = "top";
> +        textCenterPos = (boxHeight + arrowSize + radius) - boxHeight / 2;

Since this is used twice you can move the assignment after line 1679, something like:

```js
if (dimensionType === COLUMNS) {
  textCenterPos = (boxHeight + arrowSize + radius) - boxHeight / 2;
```

And when you need to switch:

```js
  textCenterPos = -textCenterPos;
```

::: devtools/server/actors/highlighters/css-grid.js:1714
(Diff revision 2)
> +        y += textCenterPos;
>        }
>      } else if (dimensionType === ROWS) {
>        if (lineNumber > 0) {
> +        boxAlignment = "left";
> +        textCenterPos = (boxWidth + arrowSize + radius) - boxWidth / 2;

Same here (using `boxWidth` of course).
Attachment #8904357 - Flags: review?(zer0) → review-
Here a couple of screenshot: in the first one we have enough space in the viewport, but the numbers are flipped (that specifically it's because the dpi issue); in the second one we don't have enough space but the numbers are not flipped.

I also forgot to mention in the review, that when the number are flipped it seems that the box's point is closest to the origin point than when they're not (basically, it seems that there is no margin).
Summary: Flip the grid line box's number position if there is not enough space along the container edge → Proper stacking of positive and negative numbers
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.
Summary: Proper stacking of positive and negative numbers → Flip the grid line box's number position if there is not enough space along the container edge
Information in comment 7 is for bug 1396673
I have not looked at the patch yet, but we should prioritize this bug now. This is getting in the way of people using the tool, and people use it more and more these days, with more and more videos and tutorials being published out there.
I like the simple approach described above of simply flipping the numbers when they are too close to the edges of the document.
Priority: P3 → P2
Yes, I really like the idea of simply flipping the numbers as well. The pointed arrow box makes this work really well. Can we do this very soon? People are starting to write and complain about not being able to see the numbers when a Grid is near the edge of the page.
I'd like to another stab at this one if no one else will be working on it in the meantime.
I'd like to take another stab at this one if no one else will be working on it in the meantime.
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review216152

I like this a lot. Thank you for picking this up!
There's one thing I find a little odd and I think a bit of work is needed there:

when the grid's first line is exactly at the edge of the document, then the arrow boxes are offset by a few pixels inside the grid, which looks nice. If, however, the grid isn't exactly at the edge of the doc (like a few pixels in) but is still too close to the edge for the arrow box to be dispayed outside the grid, then the offset isn't there.

Try on this example:
data:text/html,<style>.one, .two{display:grid;grid-template-rows:repeat(5, 1fr);grid-template-columns:repeat(5,1fr);position:absolute;}.one{top:0;right:0;left:0;height:50vh;}.two{top:50vh;left:20px;right:20px;bottom:0;}</style><div class="one"></div><div class="two"></div>

select the div.one, display the highlighter and line numbers,
look at the positive row line numbers only, they look OK, right?
Now, change top:0px to top:1px; instead in the Rules view.
You should see suddenly the arrow boxes jump to a different position altogether and the tips aren't completely visible anymore.

You can still read the numbers, but it doesn't look as good. I wish the offset would stay in this case too, so it's consistent.

Now, I can see a second minor issue.
Using the same test page, highlight the div.one grid and look at the negative row line numbers. As you can see, there's no offset here at all. Same at the bottom for negative column line numbers.
We should have consistent offset here too and the arrow boxes should not cross the grid's edge.

Oh, and one final thing.
If you look at the positive row line number 1, you can see it doesn't really point to the line, the offset is applied to this box both vertically and horizontally. Horizontally, it's expected. But it shouldn't also be offset vertically, otherwise the tip of the arrow isn't aligned with the line it points to.

::: devtools/server/actors/highlighters/css-grid.js:1190
(Diff revision 3)
>      let margin = 2 * displayPixelRatio;
>      let arrowSize = 8 * displayPixelRatio;
>  
> +    // We choose 25 px as the minimum distance as it is a good margin distance between the document and grid container edge   
> +    // without cutting off parts of the arrow box container.
> +    let minOffsetFromEdge = 25 * displayPixelRatio;

25 should be given a name and defined as a const at the top of the file.
Something like:

```
// 25 is a good margin distance between the document
// and the grid container edge without cutting off 
// parts of the arrow box container.
const OFFSET_FROM_EDGE = 25;
```

and here you'd therefore do:

```
let minOffsetFromEdge = OFFSET_FROM_EDGE * displayPixelRatio;
```

::: devtools/server/actors/highlighters/css-grid.js:1196
(Diff revision 3)
> +    let height = this.win.document.documentElement.offsetHeight;
> +    let width = this.win.document.documentElement.offsetWidth;

I'm a little bit concerned with this. Here, getting the dimensions will force the layout engine to do a sync reflow. So, there's a chance that this degrades the performance of the highlighter. Especially because we update the highlighter in a requestAnimationFrame loop.

Looking at the code, it looks like we could use `let { width, height } = this._winDimensions;` instead.

This is being defined at every refresh too, but we attempt to do it in a way that doesn't always require a reflow. And since it's already done, using these values is free. It won't cause yet another reflow.

Also, I realize that these values might actually be better for our use case: they give us the total document size, not just the visible portion of the viewport. 
With your approach for instance, if you highlight div.stripes on this page: https://stripe.com/connect
then the negative row line numbers are flipped, although they don't need to be because you can easily scroll down to see them.
Attachment #8904357 - Flags: review?(pbrosset)
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review216152

Thank you for the review!

For the positive row line number 1 being offset vertically, I will have to take a look at this a little longer. It seems to only happen if the grid edge is aligned perfectly with the edge of the document (top: 0) . I am not sure why.

I was also wondering how we want to handle the corner arrow boxes. You have probably noticed they are partially cutoff if the grid corner is close to the document's edge, so the line numbers are not completely visible. 

In the meantime, I will push another patch for review. It addresses the concerns you mentioned earlier.
(In reply to Micah Tigley [:micah] from comment #15)
> I was also wondering how we want to handle the corner arrow boxes. You have
> probably noticed they are partially cutoff if the grid corner is close to
> the document's edge, so the line numbers are not completely visible. 
Yes I see this, and for this first patch, I don't think this is a problem. You are fixing the major problem here, which is flipping the numbers so they can (mostly) be visible. That's already a big improvement.
Let's look at moving the corner numbers further in later, because that will require more thinking.
> In the meantime, I will push another patch for review. It addresses the
> concerns you mentioned earlier.
Thank you. Taking a lot at it now.
(In reply to Micah Tigley [:micah] from comment #15)
> For the positive row line number 1 being offset vertically, I will have to
> take a look at this a little longer. It seems to only happen if the grid
> edge is aligned perfectly with the edge of the document (top: 0) . I am not
> sure why.
I found out that this behaviour comes from these lines of code:
https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/devtools/server/actors/highlighters/css-grid.js#1171-1174
I don't understand why this code is here. The variable `padding` is for the text only, it's to center the text into the arrow container. So I don't understand why we would also use this padding to move the x/y coordinates of the container in some cases.
Removing these lines fixes this minor issue, but I have no idea whether removing them would regress some other feature. Do you know?
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review217080

I only have very minor comments about these last changes. 
I tested locally and it works pretty nicely. See my previous comment about the small offset. I think we should fix this and then should be good to land this.

::: devtools/server/actors/highlighters/css-grid.js:73
(Diff revisions 3 - 4)
>  const GRID_GAP_PATTERN_HEIGHT = 14; // px
>  const GRID_GAP_PATTERN_LINE_DASH = [5, 3]; // px
>  const GRID_GAP_ALPHA = 0.5;
>  
> +// 25 is a good margin distance between the document
> +// and the grid container edge without cutting off 

nit: mozreview tells me there's some empty whitespaces on this line. Can you please remove it. 
You can also configure your text editor to trim whitespaces at the end of lines automatically on save.

::: devtools/server/actors/highlighters/css-grid.js:1181
(Diff revisions 3 - 4)
>      if (!this.hasNodeTransformations) {
>        x = Math.max(x, padding);
>        y = Math.max(y, padding);
>      }
>  
>      // Draw a bubble rectanglular arrow with a border width of 2 pixels, a border color

nit: typo rectanglular.

::: devtools/server/actors/highlighters/css-grid.js:1214
(Diff revisions 3 - 4)
>          // If there is not enough space for the box number, flip its alignment and
>          // its text position.
>          if (y <= minOffsetFromEdge) {
>            boxAlignment = "bottom";
>            textCenterPos = -((boxHeight + arrowSize + radius) - boxHeight / 2);
> +  

nit: mozreview tells me there's some empty whitespaces on this line. Can you please remove it. 
You can also configure your text editor to trim whitespaces at the end of lines automatically on save.

::: devtools/server/actors/highlighters/css-grid.js:1262
(Diff revisions 3 - 4)
>  
>          if (x <= minOffsetFromEdge) {
>            boxAlignment = "right";
>            textCenterPos = -((boxWidth + arrowSize + radius) - boxWidth / 2);
> +
> +          // See comments above.

Which one? Please be a bit more explicit in this comment.

::: devtools/server/actors/highlighters/css-grid.js:1282
(Diff revisions 3 - 4)
>  
> +        // See above comment for flipping negative numbers .
>          if (x / displayPixelRatio >= width * .95) {
>            boxAlignment = "left";
>            textCenterPos = -((boxWidth + arrowSize + radius) - boxWidth / 2);
> +          

nit: mozreview tells me there's some empty whitespaces on this line. Can you please remove it. 
You can also configure your text editor to trim whitespaces at the end of lines automatically on save.
Attachment #8904357 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #18)
> I found out that this behaviour comes from these lines of code:
> https://searchfox.org/mozilla-central/rev/
> cf149b7b63ff97023e28723167725e38cf5df757/devtools/server/actors/highlighters/
> css-grid.js#1171-1174
> I don't understand why this code is here. The variable `padding` is for the
> text only, it's to center the text into the arrow container. So I don't
> understand why we would also use this padding to move the x/y coordinates of
> the container in some cases.
> Removing these lines fixes this minor issue, but I have no idea whether
> removing them would regress some other feature. Do you know?

I don't know either. I played around with transformations and the number text and arrow box pointers don't seem to be negatively affected if I remove those lines.
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review217080

Thank you for the review!

This patch removes the lines of code you previously commented on. But as I mentioned above, I made sure to play around with transformations in the rule view. I couldn't find anything that negatively affected how the line numbers are displayed.

This patch also fixes the linting errors you brought up.
Comment on attachment 8904357 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/176154/#review217432
Attachment #8904357 - Flags: review?(pbrosset) → review+
Comment on attachment 8943147 [details]
Bug 1396666 - Flip the grid line box's number position if there is not enough space along the container edge.

https://reviewboard.mozilla.org/r/213454/#review219204

Just fixed the ESLint failures on Michah's patch, kept her as the author.
Let's land this.
Attachment #8943147 - Flags: review?(pbrosset) → review+
Attachment #8904357 - Attachment is obsolete: true
hg error in cmd: hg rebase -s 8f270090b35c253b687968f0affcbad7fc19eb90 -d 0d52b29a58e7: abort: can't rebase public changeset 8f270090b35c
(see 'hg help phases' for details)
(In reply to Mozilla Autoland from comment #27)
> hg error in cmd: hg rebase -s 8f270090b35c253b687968f0affcbad7fc19eb90 -d
> 0d52b29a58e7: abort: can't rebase public changeset 8f270090b35c
> (see 'hg help phases' for details)
wat ... oO
I don't know how this could happen. The changeset I'm asking autoland to apply isn't public yet.
Going to mark this as checkin-needed for someone to take a look, as I don't really know what to do.
Keywords: checkin-needed
hg error in cmd: hg rebase -s 8f270090b35c253b687968f0affcbad7fc19eb90 -d 0fd485f3f388: abort: can't rebase public changeset 8f270090b35c
(see 'hg help phases' for details)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/215da04e16be
Flip the grid line box's number position if there is not enough space along the container edge. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/215da04e16be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.