Closed Bug 1369942 Opened 7 years ago Closed 7 years ago

Display Negative Line Numbers in CSS Grid Inspector

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: jensimmons, Assigned: micah)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

At the moment, The CSS Grid Inspector displays a single line number for each line — the positive number, and if there are more than one positive number for that line, only the larger one is visible. Each line on an explicit Grid actually has two numbers — a positive number, and a negative number. Each line on an implicit Grid only has one number. And any line could be stacked on top of other lines, by effect appearing to be one line with multiple positive and negative numbers. To get this right, we should display all numbers, not just a single positive number.
I just did a sketch here of one possible evolution of the direction in which we are already going. https://monosnap.com/file/FJAy02xXnCiwzQs7EFKjOEwem4SEiV.png On the left is a screenshot of the current tool (as of June 2, 2017). On the right is a proposed solution. Note that the row line numbers are numbered 1 2 3 5 6 in the current status screenshot. That's because line 4 and line 5 are in the exact same place, and the number '4' and '5' are stacking on top of each other with the bigger number on top (I presume -- maybe technically the '4' isn't being rendered). This can be confusing for authors. Someone commented on another ticket that there's a bug that mis-numbers the lines —- because they didn't understand that Grid can stack lines on top of each other.
I'm not completely sure about running the negative numbers along the grid container's block-end and inline-end edges, but the reason I did that is because of another aspect of the tool.... When you have a small grid of only one or two columns/rows, the lines and numbers can suddenly seem really weird. https://monosnap.com/file/qSW5C5aFGSk0fsAnYh6xSS7fJ2YLzu.png I ran into this one day while working with this example for the first time in a long time. Without having numbers down both sides of the container, it was confusing — what I was looking at. Putting the negative numbers there might help. Need to test to know for sure.
I think it would be good to add another setting to the Layout Panel. Right now we have a toggle for: [] Display numbers on lines We could add a second to let people show all numbers, or just the positive numbers. That way when authors are not using the negative numbers, they can turn just those off to reduce clutter: [] Display positive numbers on lines [] Display negative numbers on lines Or: [] Display numbers on lines [] Hide negative numbers Or something else.
Priority: -- → P3
After working through the usecases where the first line is not line '1' (in #1373678), I believe showing the negative numbers is pretty important and we should get to it soon. I also think we need to move the number labels so that the first line (row & column) has two labels, not one in the corner. I posted a drawing that moves the labels away from the edges of the container in comment #2 above. I now prefer, however, that the negative and positive numbers be on the same side of the container, not the opposite side. I'll do some drawings of options as soon as I can. I just got off of a plane, though, and need to reset my life / go run a meetup / get something to eat. So.... soon. Also :D
Adding a comment here to don't forget to tackle https://bugzilla.mozilla.org/show_bug.cgi?id=1373678#c40 in this bug – since the part mentioned there will be refactored here anyway.
See Also: → 1373678
(In reply to Jen Simmons [:jensimmons] from comment #1) > I just did a sketch here of one possible evolution of the direction in which > we are already going. > > https://monosnap.com/file/FJAy02xXnCiwzQs7EFKjOEwem4SEiV.png I like it, but the problem with this approach is that it doesn't fit well when a transformation is applied: if you try on the current Grid Inspector to rotate, scale and skew a grid, you'll see how the line's number are properly aligned, and it's hard to do the same thing with this mockup. We need to do something that scales well with transformation applied too, the hardest part IMVHO are the overlap for row / column on the corner. Also, that increase the space needed around the grid – and if there is no enough space, e.g. is positioned at the margin of the document, we need to display the numbers inside the grid, makes the column and row corner's number overlaps again. For similar reason (space available and transformation) I wouldn't use the solution for line 4,5 and -2, -3 (the collapsed lines). We can maybe use a different box to show the stack. I will give some thoughts about it, but I doubt will be able to cover every scenarios perfectly, it's likely we'll have to compromise.
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Comment on attachment 8886022 [details] Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. https://reviewboard.mozilla.org/r/156800/#review162892 Clearing review because of the new prototypes requested
Attachment #8886022 - Flags: review?(gl)
Attachment #8886022 - Flags: review?(gl) → review?(zer0)
zer0, we have broken up your proposal to many smaller bits. This one simply implements the negative line number display behind a pref (that will eventually be removed). This will continue into a series of other incremental changes that will implement: 1. Scrolling of positive line numbers. 2. Scrolling of negative line numbers. 3. Line number alignments at viewport edges. 4. Different colours for the line number boxes. 5. Row/column directional arrows
What is "scrolling of positive and negative line numbers"?
(In reply to Jen Simmons [:jensimmons] from comment #11) > What is "scrolling of positive and negative line numbers"? We are currently prototyping 2 approaches - (1) Negative line numbers beside the positive line numbers, and (2) Negative line numbers on opposite side. Scrolling of positive/negative line numbers would mean the line numbers would scroll with the viewport if the grid is bigger than the current viewport. I will attach a gif to demonstrate. We felt this made it very compelling for the negative line numbers on the opposite side because it means it would always be visible. This approach would also address some of the issues brought up in https://github.com/ZER0/grid-negative-numbers/wiki.
Ah, yes — the image you sent me shows clearly what you mean. This is called a "sticky header" in the web industry, where the info in the header sticks to the edge of the viewport as a user scrolls. It's a good idea for our tool.
Comment on attachment 8886022 [details] Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. https://reviewboard.mozilla.org/r/156800/#review164124 Thanks for working on this! The alignment and the position of the negative numbers doesn't appear correct, and that's why the r-. I will attach a screenshot and also a reference to the live example so that you can test your changes.
Attachment #8886022 - Flags: review?(zer0) → review-
Let's start from the easy part: 1. As you can see from the screenshot, the negative numbers seems misaligned. You can see from the screenshot the -5's box for example, on the right: it should be aligned as the 1's box on the left. Also the -6's box on the bottom, should be aligned as the 1's box on the top. The rest seems be properly aligned! 2. The negative numbers are not correct. They basically should start from -1 from to right to left, and from the bottom to the top. In the patch they start from -3. here the annotated png made by Jen that shows how they should looks like: https://monosnap.com/file/htG1YbfLegYxdtR3gfyiHgNP7PemSV.png (The annotation shows the negatives over the positive, of course we want them on bottom and on the right) And here the live example where you can test the patch over it: https://s.codepen.io/jensimmons/debug/GENbzL (further details on bug 1373678) Also here there is an example with the previous patch (but still, the wrong numbers) and how they should looks like: https://github.com/ZER0/grid-negative-numbers/wiki#current-patch-actual-screenshots Hope it helps! Let me know if you have any questions.
Thanks for the review! I implemented the changes you suggested and I've changed it up so that the positioning of the grid line numbers are a little away from the grid edges. I noticed that was the way they were drawn in your mockup for negative line numbers: https://github.com/ZER0/grid-negative-numbers/wiki#how-we-could-handle-those-issues-with-negative-numbers. Here are two gifs that demonstrate line numbers with transformations applied: https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/LineNumbersCenteredAwayFromEdge_1.gif https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/LineNumbersCenteredAwayFromEdge_2.gif Alternatively, I also have another patch where it keeps the grid line numbers centered on the edge: https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/LineNumbersCenteredOnEdges_1.gif https://raw.githubusercontent.com/TigleyM/firefox-devtools-patches/master/demos/NegativeLineNumbers/LineNumbersCenteredOnEdges_2.gif You'll notice for the last gif the grid line numbers are overlapping since the corner grid line numbers are offset. But since the mockup has it so that the grid line numbers are away from the edge, we probably don't need to worry about this.
Blocks: 1383327
Blocks: 1383329
Attachment #8886022 - Attachment is obsolete: true
Comment on attachment 8888641 [details] Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. https://reviewboard.mozilla.org/r/159666/#review166034 ::: devtools/server/actors/highlighters/css-grid.js:1160 (Diff revision 2) > + } > + } > + } > + > +/** > + * Render the negative grid lines given the grid dimension information of the This JSDoc needs to be indented correctly. ::: devtools/server/actors/highlighters/css-grid.js:1163 (Diff revision 2) > + > +/** > + * Render the negative grid lines given the grid dimension information of the > + * column or row lines. > + * > + * see @param for renderLines. s/see/See ::: devtools/server/actors/highlighters/css-grid.js:1170 (Diff revision 2) > + renderNegativeLineNumbers(gridDimension, dimensionType, mainSide, crossSide, > + startPos) { > + let lineStartPos = startPos; > + > + // We still want to display negative line numbers even if the numbers are zero. > + // Create an array of new grid lines except they are numbered according to their s/except they/that ::: devtools/server/actors/highlighters/css-grid.js:1525 (Diff revision 2) > + > x = linePos + breadth / 2; > y = startPos; > } else { > + if (Services.prefs.getBoolPref(NEGATIVE_LINE_NUMBERS_PREF)) { > + // If the line number is negative, offset it right by its s/right/to the right
Comment on attachment 8888641 [details] Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. https://reviewboard.mozilla.org/r/159666/#review166036 ::: devtools/server/actors/highlighters/css-grid.js:1532 (Diff revision 2) > + if (lineNumber < 0) { > + startPos += (boxWidth + 2) / devicePixelRatio; > + } else { > + startPos -= (boxWidth + 2) / devicePixelRatio; > + } > + } Should also add a new line after this if statement block
Comment on attachment 8888641 [details] Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. https://reviewboard.mozilla.org/r/159666/#review166956 Looks great, thanks! Just few comments to reduce the code. ::: devtools/server/actors/highlighters/css-grid.js:1173 (Diff revision 3) > + > + // We still want to display negative line numbers even if the numbers are zero. > + // Create an array of new grid lines that are numbered according to their > + // index + 1. This way we can correctly display the negative line numbers for grids > + // that may have line numbers marked as 0. > + let negativeLineNumbers = []; Do we really need the `negativeLineNumbers` array and the double loop? Maybe we can use directly `gridDimension.lines`: ```js let { lines } = gridDimension; // I just used this loop, but if it feel akward use whatever you're comfortable with, to iterate the lines. for (let line, i = 0; line = lines[i++];) { this.renderGridLineNumber(i - lines.length, line.start, lineStartPos, line.breadth, dimensionType); } ``` Or something like that, unless I misread something. ::: devtools/server/actors/highlighters/css-grid.js:1511 (Diff revision 3) > // Calculate the x & y coordinates for the line number container, so that it is > // centered on the line, and in the middle of the gap if there is any. > let x, y; > > if (dimensionType === COLUMNS) { > + if (Services.prefs.getBoolPref(NEGATIVE_LINE_NUMBERS_PREF)) { It seems this block is common to both `COLUMNS` AND `ROWS` so it might be put before the `if`: ```js let startOffset = (boxHeight + 2) / devicePixelRatio; if (lineNumber < 0) { startPos += startOffset; } else { startPos -= startOffset; } if (dimensionType === COLUMNS) { x = linePos + breadth / 2; y = startPos; } else { x = startPos; y = linePos + breadth / 2; } ```
Attachment #8888641 - Flags: review?(zer0) → review+
Attached image overlapping corners
(In reply to Micah Tigley [:micah] from comment #16) A couple of additional things – no blocking this patch in anyway: > Alternatively, I also have another patch where it keeps the grid line > numbers centered on the edge: Really good, thanks! Keep this patch! It could be useful to play with, maybe once we have the arrows (bug 1383327) and the different colors, to check what's better in different conditions. Maybe if you can add the patch's code to this bug as future reference it would be great! > You'll notice for the last gif the grid line numbers are overlapping since > the corner grid line numbers are offset. But since the mockup has it so that > the grid line numbers are away from the edge, we probably don't need to > worry about this. Yes, I noticed that. This is also happening with this patch when the grid is small (see the attachment), but i think we should address this in a separate bug.
I made this still from the animated gifs Micah's been making. Dropping URL here for easy access. https://monosnap.com/file/KklUVcwffQA4kVXLbxGOdFkBKYyCjy.png I'm still not sure putting negative line numbers on the opposite side is best. But I'm also not sure that it's not best..... I'll be excited to have this running in Nightly, so I can see how it feels, and test it with various examples. I also put the question to the CSS Layout Club Slack, showing them stills and gifs... no feedback yet. I'm very happy this is happening. I'm seeing people on Twitter get confused about what the Grid negative numbers do, where they go, why they exist. Having even a half-working implementation of negative line numbers will help developers a lot! It'll be an Even More Compelling reason to use Firefox Nightly. (Oh, in reference to comment 23 from Matteo — I really do not think that making the negative numbers a different color is a good idea. I explained my thoughts here: https://bugzilla.mozilla.org/show_bug.cgi?id=1383329, and far as I know, that idea is either being scrapped, or is on hold for further longer design consideration. It was a good idea to consider, but as a person who's written a ton of code using CSS Grid, I think having two colors for each grid adds as much confusion as it solves. Everything for each grid should be one color. But of course, all that is off-topic for this ticket. We can discuss this on #1383329)
For future reference: here's the patch for displaying line numbers whose box containers are centered on the grid container edge.
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #22) > Comment on attachment 8888641 [details] > Bug 1369942 - Display Negative Line Numbers in CSS Grid Inspector. > > https://reviewboard.mozilla.org/r/159666/#review166956 > > Looks great, thanks! Just few comments to reduce the code. > > ::: devtools/server/actors/highlighters/css-grid.js:1173 > (Diff revision 3) > > + > > + // We still want to display negative line numbers even if the numbers are zero. > > + // Create an array of new grid lines that are numbered according to their > > + // index + 1. This way we can correctly display the negative line numbers for grids > > + // that may have line numbers marked as 0. > > + let negativeLineNumbers = []; > > Do we really need the `negativeLineNumbers` array and the double loop? Maybe > we can use directly `gridDimension.lines`: > > ```js > let { lines } = gridDimension; > // I just used this loop, but if it feel akward use whatever you're > comfortable with, to iterate the lines. > for (let line, i = 0; line = lines[i++];) { > this.renderGridLineNumber(i - lines.length, line.start, lineStartPos, > line.breadth, dimensionType); > } > ``` > > Or something like that, unless I misread something. > > ::: devtools/server/actors/highlighters/css-grid.js:1511 > (Diff revision 3) > > // Calculate the x & y coordinates for the line number container, so that it is > > // centered on the line, and in the middle of the gap if there is any. > > let x, y; > > > > if (dimensionType === COLUMNS) { > > + if (Services.prefs.getBoolPref(NEGATIVE_LINE_NUMBERS_PREF)) { > > It seems this block is common to both `COLUMNS` AND `ROWS` so it might be > put before the `if`: > > ```js > let startOffset = (boxHeight + 2) / devicePixelRatio; > > if (lineNumber < 0) { > startPos += startOffset; > } else { > startPos -= startOffset; > } > > if (dimensionType === COLUMNS) { > x = linePos + breadth / 2; > y = startPos; > } else { > x = startPos; > y = linePos + breadth / 2; > } > ``` I agree with this. We definitely didn't need the extra loop for calculating the number of negatives and there was definitely some DRY going on in that last bit. Thank you for the refactoring advice :)
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b51e5b499c Display Negative Line Numbers in CSS Grid Inspector. r=zer0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: