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)
DevTools
Inspector
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.
Assignee | ||
Updated•7 years ago
|
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Severity: normal → enhancement
Priority: -- → P3
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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 5•7 years ago
|
||
mozreview-review |
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-
Comment 6•7 years ago
|
||
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).
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 8•7 years ago
|
||
Information in comment 7 is for bug 1396673
Comment 9•6 years ago
|
||
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
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
I'd like to another stab at this one if no one else will be working on it in the meantime.
Assignee | ||
Comment 12•6 years ago
|
||
I'd like to take another stab at this one if no one else will be working on it in the meantime.
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Whiteboard: [designer-tools]
Comment 17•6 years ago
|
||
(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.
Comment 18•6 years ago
|
||
(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 19•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 20•6 years ago
|
||
(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.
Assignee | ||
Comment 21•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 23•6 years ago
|
||
mozreview-review |
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 24•6 years ago
|
||
Pushed this commit to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d46744a0f5d83979a558a77969995441e4b157
Blocks: 1430916
Comment hidden (mozreview-request) |
Comment 26•6 years ago
|
||
mozreview-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+
Updated•6 years ago
|
Attachment #8904357 -
Attachment is obsolete: true
Comment 27•6 years ago
|
||
hg error in cmd: hg rebase -s 8f270090b35c253b687968f0affcbad7fc19eb90 -d 0d52b29a58e7: abort: can't rebase public changeset 8f270090b35c (see 'hg help phases' for details)
Comment 28•6 years ago
|
||
(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
Comment 29•6 years ago
|
||
hg error in cmd: hg rebase -s 8f270090b35c253b687968f0affcbad7fc19eb90 -d 0fd485f3f388: abort: can't rebase public changeset 8f270090b35c (see 'hg help phases' for details)
Comment 30•6 years ago
|
||
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
Comment 31•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/215da04e16be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•