Better alignment for grid line numbers

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Inspector
P3
normal
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: zer0, Assigned: jdescottes)

Tracking

(Blocks: 1 bug, {DevAdvocacy})

unspecified
Firefox 55
DevAdvocacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

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

Attachments

(9 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8839916 [details]
grid line numbers screenshot

Currently when we display the line number for the grid's lines, we have two different alignment for vertical and horizontal lines (the horizontal are displayed after the lines, the vertical above, see the attachment).

We should be consistent and choose one type o alignment.
Also, we display the line number 1 twice (one for horizontal and one for vertical; see the attachment).
(Reporter)

Updated

a year ago
Blocks: 1181227
Flags: qe-verify+
Priority: -- → P3
Yes, please. I think the numbers should align center to center — meaning the center of the number should align with the center of the gap. The number should not touch the grid container (when the lines are not extended), but instead there should be a bit of space. The numbers should be much, much bigger. And a thick bold. And the numbers should be the same color as the Grid they describe.
QA Whiteboard: [DevRel:P1]
Keywords: DevAdvocacy
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Blocks: 1347964
Comment hidden (mozreview-request)
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

Clearing review flag for now, just wanted to attach here as a backup.
Attachment #8848592 - Flags: review?(zer0)
(Reporter)

Comment 5

a year ago
(In reply to Julian Descottes [:jdescottes] from comment #4)
> Comment on attachment 8848592 [details]
> Bug 1341612 - align grid highlighter numbers with lines and increase
> readability;
> 
> Clearing review flag for now, just wanted to attach here as a backup.

So, it's a WIP?
Do you want a feedback anyway or do you prefer waiting for the review?
Created attachment 8852108 [details]
proposal 1 : number with bigger font

This version fixes the alignment issues when we have gaps, uses a bigger font and also uses the grid color as the font color.
Created attachment 8852112 [details]
proposal 2 : number with container and background

This other version keeps a small font but adds a background and border around the number to ensure it is always readable. It also merges the 1/1 for the x/y axes.
Jen: I attached two options to improve the grid line number display. The first one (attachment 8852108 [details]) is basically the one you suggested in your feedback. The second one (attachment 8852112 [details]) is an alternative option.

We could also have a mixed approach using the background + border but still increasing the distance from the grid so that the 1's on x & y don't overlap each other.

My issue with proposal 1 is that, using a bigger font, we don't really ensure readability on all backgrounds. It also feels a bit inconsistent with our other highlighters.

Let me know which way you'd prefer to go.
Flags: needinfo?(jensimmons)
I like proposal #2.
Getting quick feedback from my user group (Slack channel), people like #2 more. 

I assume the number is on a white background, in a little block surrounded by the color line. That's a really good idea. 

It might be good to see these with different color combinations. Like on a page with a dark background. With lines that are light — or what happens when there are two Grids that are on top of each other (see http://labs.jensimmons.com/2016/examples/grid-content-1.html for a usecase). With an example that has a gap of 0. 

Also lines on the explicit Grid have two numbers each (a positive number and a negative number), so we should think about that, too. How do they fit together in the box. (Do we show the negative numbers yet?)
Flags: needinfo?(jensimmons)
Comment hidden (mozreview-request)
Thanks for the feedback! I attached the changeset behind proposal #2 here, for feedback from Matteo.

> It might be good to see these with different color combinations. Like on a page with a dark background. 
> With lines that are light

My assumption is that the user should pick a grid color which is easy to see for the grid itself. Even in case the lines are hard to see the number remains a black font on a white background so it'll always be readable.

> or what happens when there are two Grids that are on top of each other (see 
> http://labs.jensimmons.com/2016/examples/grid-content-1.html for a usecase). 
> With an example that has a gap of 0.

That's a good point. We don't support showing multiple grids at the moment. When we do, maybe we should make the line numbers toggle per grid instead of being a global setting?

> Also lines on the explicit Grid have two numbers each (a positive number and a negative number), 
> so we should think about that, too. How do they fit together in the box. (Do we show the negative numbers yet?)

We don't show the negative numbers yet, but that should be fairly easy to add with this representation, if we simply go for displaying {positive, negative} in the container.

Another thing that we don't handle at the moment are overlapping lines. Only one number is visible at the moment. Ideally, we should detect collisions between line numbers and try to make sure all line numbers are visible.
Attachment #8848592 - Flags: review?(zer0) → review?(gl)

Comment 13

a year ago
mozreview-review
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

https://reviewboard.mozilla.org/r/121500/#review128362

::: devtools/server/actors/highlighters/css-grid.js:769
(Diff revision 2)
>                       this.getFirstColLinePos(fragment),
>                       this.getLastColLinePos(fragment));
> +
> +    // Line numbers are rendered in a second step to avoid being overlapped by existing
> +    // lines.
> +    this.renderLineNumbers(fragment.cols, quad, COLUMNS, "left", "top",

It seems to me that we should be checking this.options.showGridLineNumbers here before going thorugh the fragment information to render the line numbers.
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

Passing this back to zer0
Attachment #8848592 - Flags: review?(gl) → review?(zer0)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

https://reviewboard.mozilla.org/r/121500/#review128566

Thanks for working on this, Julian! Unfortunately I have to set r- since it doesn't scale well on displays with different pixel density; I'm going to add a screenshot about it and about another issue.
Since we cannot have unit tests for such things, be sure you're manually test at least on diffent displays with different density and possibly on different platforms – the <canvas> can render / behave a bit differently depends by the platform – and different zoom level.

::: devtools/server/actors/highlighters/css-grid.js:841
(Diff revision 3)
> +   *
> +   * see @param for renderLines.
> +   */
> +  renderLineNumbers(gridDimension, {bounds}, dimensionType, mainSide, crossSide,
> +              startPos) {
> +    let lineStartPos = (bounds[crossSide] / getCurrentZoom(this.win)) + startPos;

you can store `currentZoom` since we're using it twice.

::: devtools/server/actors/highlighters/css-grid.js:935
(Diff revision 3)
>      this.ctx.font = fontSize + "px " + GRID_FONT_FAMILY;
>  
>      let textWidth = this.ctx.measureText(lineNumber).width;
>  
> +    // Approximate height of the text, in pixels.
> +    let height = GRID_FONT_SIZE * 1.5;

You're not taking the display density in account, here. The box won't scale well on different displays; you should probably use `fontSize` as base, and if you want apply some adjustment on top of it.

::: devtools/server/actors/highlighters/css-grid.js:937
(Diff revision 3)
>      let textWidth = this.ctx.measureText(lineNumber).width;
>  
> +    // Approximate height of the text, in pixels.
> +    let height = GRID_FONT_SIZE * 1.5;
> +    // Padding in pixels for the line number text inside of the line number container.
> +    let padding = 5;

Here too you have to consider the display density. This measure will be probably bigger than intendend on a regular monitor (1ddpx).

::: devtools/server/actors/highlighters/css-grid.js:957
(Diff revision 3)
> +    x = Math.max(x, padding);
> +    y = Math.max(y, height + padding);
> +
> +    // Draw a rounded rectangle with a border width of 4 pixels, a border color matching
> +    // the grid color and a white background (the line number will be written in black).
> +    this.ctx.lineWidth = 4;

Ditto.
You probably wanted the rect's line to be a bit thicker than regular grid's line. On a retina display, this value would be just twice as big; but on regular minitor you have a line that is four times thicker. This line therefore should be:

```js
this.ctx.lineWidth = 2 * displayPixelRatio;
```

::: devtools/server/actors/highlighters/css-grid.js:965
(Diff revision 3)
> +    this.roundedRect(
> +      x,
> +      y - height,
> +      textWidth + (2 * padding),
> +      height + (2 * padding),
> +      3

This value should also consider the display density – otherwise on a 1dppx monitor would be more rounded than intended, and on a higher density monitor than a retina one, it would be less rounded.

::: devtools/server/actors/highlighters/css-grid.js:989
(Diff revision 3)
> +   * @param  {Number} height
> +   *         The height of the rectangle.
> +   * @param  {Number} radius
> +   *         The radius of the rounding.
> +   */
> +  roundedRect(x, y, width, height, radius) {

This function shouldn't belong to the grid highlighter, is too generic. It doesn't take any argument that is relative to a grid element, and it doesn't consume anything from the `this` object rather than the drawing context.
We should have it as local function of this module, without export it, and make the first argument the drawing context. We could create an external utility module for canvas in the future, if we're seeing that we're using a lot of those functions and / or we're creating another highlighter based on <canvas> element that uses such functions.
Attachment #8848592 - Flags: review?(zer0) → review-
(Reporter)

Comment 17

a year ago
Created attachment 8854346 [details]
Screenshot of grid on 1dppx display

Here how the patch behave on a regular non retina display.
(Reporter)

Comment 18

a year ago
Created attachment 8854348 [details]
Overlapping on zoom

Here another issue: if the user zoom enough, the number 1 for column and row are likely to overlap, if the grid is close to the corner's page. That potentially could happens to others number as well, but this scenario is common – since it doesn't depends by the grid's width / height.

I don't think is a big issue; we could probably detect such collision and display only one number as we do for the "non infinite" version, but I'm not sure (Now, with the box, we also shows a kind of "stacking" effect).

Jen, what do you think?
Flags: needinfo?(jensimmons)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

https://reviewboard.mozilla.org/r/121500/#review128566

Thanks for the review. I tested the new patch on a non-retina display and it looks good IMO. Regarding the overlapping issue, I'm not sure we can do much about it ... Maybe using a slightly transparent background instead of pure white ? This way the user might be able to guess which number is "below".

> You're not taking the display density in account, here. The box won't scale well on different displays; you should probably use `fontSize` as base, and if you want apply some adjustment on top of it.

It's difficult to know what to use to calculate the text height here ... Measuring the width of "m" seems ok for the current font.

> Ditto.
> You probably wanted the rect's line to be a bit thicker than regular grid's line. On a retina display, this value would be just twice as big; but on regular minitor you have a line that is four times thicker. This line therefore should be:
> 
> ```js
> this.ctx.lineWidth = 2 * displayPixelRatio;
> ```

For the record it's twice as big because the "fill" will overlap with half of the line. Visually it should look like it has the same size as the grid lines.

> This function shouldn't belong to the grid highlighter, is too generic. It doesn't take any argument that is relative to a grid element, and it doesn't consume anything from the `this` object rather than the drawing context.
> We should have it as local function of this module, without export it, and make the first argument the drawing context. We could create an external utility module for canvas in the future, if we're seeing that we're using a lot of those functions and / or we're creating another highlighter based on <canvas> element that uses such functions.

For now, I agree that just a separated method is fine.
(Reporter)

Comment 21

a year ago
mozreview-review
Comment on attachment 8848592 [details]
Bug 1341612 - align grid highlighter numbers with lines and increase readability;

https://reviewboard.mozilla.org/r/121500/#review129348

All my comments are addressed, thanks! It looks good! You might need to rebase this patch since bug 1345434 is landed (at least in autoland, I guess).
Attachment #8848592 - Flags: review?(zer0) → review+
Comment hidden (mozreview-request)
Rebase done! Landing.

Comment 24

a year ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a13a064593f9
align grid highlighter numbers with lines and increase readability;r=zer0

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a13a064593f9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8854348 [details]
Overlapping on zoom

It seems like the overlapping won't happen on zoom if it's not happening before the zoom starts — everything zooms together. 

But meanwhile, we do need a plan for overlapping. Most lines have more than one number. And once we start adding names, most lines have multiple names. A design system would figure this out — how to handle a bunch of labels all at the same time. 

I don't have a quick answer. :/ I wish I did. 

I guess in the meanwhile, we ... does hovering over one number bring it to the front? As a user I would expect something like that. If I hover over the [1] that's in the front, it stays there. If I hover over the [1] that's in the back, it comes to the front. If that happens now, cool. If that requires a lot of engineering to build such interactivity, then I'd say let's design a comprehensive solution to all the names and line numbers fitting into available space, and potentially overlapping. And if coming forward on hover seems like a good idea, build such a thing then. Not now.
Flags: needinfo?(jensimmons)
Can you please help me with some steps, in order to be able to verify this? Thanks
The goal here is to check that grid line numbers are visible&readable in various configurations.

First of all, enable the layout panel (either set `devtools.layoutview.enabled` to true in about:config, or check "Enable layout panel" in the devtools preferences Inspector section).

- go to a page using CSS grid (eg: http://labs.jensimmons.com/2017/01-003.html)
- open inspector -> layout panel 
- enable the `Overlay grid` for one of the available grid containers
- check "display numbers on lines"

Verify that the numbers are centered on the grid lines and reabable.
Try with and without "Extend grid lines infinitely"
Feel free to try and change the grid-related properties in the inspector to check that the line numbers still behave correctly.
For instance, the line number should remain centered inside of the gap in case there is one.

There are some known limitations with the current implementation: if two lines somehow overlap, we don't have any mechanism to display both line numbers, so only the second one is displayed. But feel free to log any issue you might see. We will most likely use this as a basis to log follow up bugs.

Let me know if I can help further with the testing, thanks!
Hi Julian,

Thanks for the steps, I did some exploratory on this issue using FF Nightly 55.0a1(2017-05-16) on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 x64 and I didn't find any issue. I do have a question related to the last example from the test page, the overlapping & ratio-based array, I saw that the distance from the numbers is not equal, the distance should be equal or this is the expected behavior? 
If the above question comes out like this is the expected behavior I will mark this as a verified fix.
Flags: needinfo?(jdescottes)
Created attachment 8868478 [details]
grid_numbers_last_example.jpg

Here is what I see for the last example. In my opinion this is correct. As long as the numbers are displayed on the lines of the grid, then it is correct. 

The distance between the grid lines can vary, so if that's what you meant, then yes it's ok.

If you see something significantly different from my screenshot, let's get in touch on IRC.
Flags: needinfo?(jdescottes)
Created attachment 8868520 [details]
Recording #37.mp4

The attached file with the gap between the image and the line if the browser is zoomed in.
Created attachment 8868521 [details]
Screen Shot 2017-05-17 at 2.53.38 PM.png

This is the correct attachment for the gap between the image and the line if the browser is zoomed in.
Attachment #8868520 - Attachment is obsolete: true
Thanks for testing!

Matteo: are we tracking the alignment issues between the grid and the content as illustrated in the screenshot. It seems like they are easy to trigger with the 3rd grid container of http://labs.jensimmons.com/2017/01-003.html, after zooming in with the browser.
Flags: needinfo?(zer0)
Created attachment 8868533 [details]
Number is not correct.png

I saw another issue that is related to this, here is the scenario:

1. Go to a page using CSS grid (eg: http://labs.jensimmons.com/2017/01-003.html)
2. Open inspector -> layout panel 
3. Enable the `Overlay grid` for one of the available grid containers
4. Check "display numbers on lines"
5. Zoom out the browser until 30%
6. Select the third grid "Responsive" 

Please see the last numbers, after 20 you will see 38. I attached a print screen for a better understanding. 
Please tell me your opinion about this.
Re: #34

It's not that the numbers are incorrect, it's that there are a whole pile of lines stacked on top of each other, with a whole bunch of numbers stacked on top of each other, and you are only seeing the last number. For example, in your screenshot, the lines are numbered ... 15, 16, 17, 18, 19, 20, 38. Or so it appears. Line "38" is actually also line 21, 22, 23, 24... 36, 37, and 38. Yes, there are 18 lines on top of each other. 

I've filed an issue for adding the missing line numbers here: https://bugzilla.mozilla.org/show_bug.cgi?id=1369942
(Reporter)

Comment 36

a year ago
(In reply to ovidiu boca[:Ovidiu] from comment #32)

> This is the correct attachment for the gap between the image and the line if
> the browser is zoomed in.

(In reply to Julian Descottes [:jdescottes] from comment #33)
> Thanks for testing!
> 
> Matteo: are we tracking the alignment issues between the grid and the
> content as illustrated in the screenshot. It seems like they are easy to
> trigger with the 3rd grid container of
> http://labs.jensimmons.com/2017/01-003.html, after zooming in with the
> browser.

~1px it *might* happen with several zoom level applied (in or out) since the rounding; but the patch on bug 1351997 should at least mitigate the issue if not fix at all.
Flags: needinfo?(zer0)
Hi Julian,

I can confirm this issue is fixed, I verified using Firefox 55.0b4 (buildID: 20170622104007) on Win 8.1 x64, Mac 10.12 and Ubuntu 14.04 x64.
I didn't  see any significantly different from your screenshot (comment 30).
If you also agree with me, please let me known, to change the tracking flags, to verified fixed.

Thanks, 
       Timea
Flags: needinfo?(jdescottes)
Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(jdescottes)
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.