1452503 - Implicit and Explicit Grid Lines are drawn with a close path and are no longer showing their dotted pattern

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[kev.landry]

Attachment: firefox_2018-04-08_21-36-26.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180404171943

Steps to reproduce:

Open devtool, visualize a grid container then go to Layout then click to div.container then click Extends lines infinitely.


Actual results:

I don't see anymore implicite and explicite dashed tracks, only solid tracks.


Expected results:

We should still see implicite and explicite tracks and by that i mean doted and dashed tracks.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Firefox: 61.0a1, Build ID: 20180410100115

I have managed to reproduce this issue on latest Nightly (61.0a1) build on Windows 7 x64. In order to reproduce the issue I have used the following test case: https://www.w3schools.com/css/tryit.asp?filename=trycss_grid_layout_named

The issue is not reproducible on latest Firefox 59.0.2. Considering this I have found the regression window using the mozregression tools. Here are the results:

Last good revision: 32c5d61d51ed33a32cd8e7407e60def0108fac8d
First bad revision: 7939ec78670514e4fb273cc77af89f28eadbcbfa
Pushlog: https://goo.gl/fP6dsf

Looks like bug 1432036 introduce this issue. Shuoyi can you please take a look at this issue?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Gabriel, you reviewed the regressing patch, could you help?

Comment 3

[Patrick Brosset <:pbro>]

I looked at the bug that regressed this. It probably comes from this change:

+  ctx.beginPath();
   ctx.moveTo(Math.round(x1), Math.round(y1));
   ctx.lineTo(Math.round(x2), Math.round(y2));
+  ctx.closePath();

This is a pretty important feature of the grid highlighter, so we should try to fix this. Especially because it seems simple enough. 
We have a few days to land this in 61, and then uplift in 60.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1452503 - Fix the implicit and explicit lines drawn on the grid highlighter.

Review commit: https://reviewboard.mozilla.org/r/240230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240230/

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8971474 [details]
Bug 1452503 - Fix the implicit and explicit lines drawn on the grid highlighter.

https://reviewboard.mozilla.org/r/240230/#review246058

I don't know the canvas API well enough to understand the implications of beginning a path (in drawLine) and not closing it.
We use this drawLine method in other places too, I'm not sure what impact this will have there.

Hopefully Julian can be a better reviewer than me on this patch, since he's worked with canvas quite a lot.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8971474 [details]
Bug 1452503 - Fix the implicit and explicit lines drawn on the grid highlighter.

https://reviewboard.mozilla.org/r/240230/#review246066

This looks ok to me, although the APIs in utils/canvas are pretty inconsistent making them hard to use and understand :)
Not calling closePath is not an issue. It's a way to go back automatically to the beginning of the path, but it's not mandatory at all. 

For a follow-up: 100% of the consumers of canvas/utils::drawLine call ctx.stroke() right after calling drawLine. 
I think this call to stroke() would make more sense in drawLine.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a88bd1be71
Remove closePath call to fix the implicit and explicit lines drawn on the grid highlighter. r=jdescottes

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Needinfo myself to uplift.

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/74a88bd1be71

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8971474 [details]
Bug 1452503 - Fix the implicit and explicit lines drawn on the grid highlighter.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1432036
[User impact if declined]: The grid highlighter loses the ability to distinguish implicit and explicit lines for CSS grids.  
[Is this code covered by automated tests?]: No, the highlighter is written using the Canvas APIs.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
(1) Go to https://gridbyexample.com/examples/code/example10.html
(2) Turn on the grid highlighter from the Layout panel for div.wrapper
(3) You should see various different line styles that covers edges of explicit grid, inner explicit grid lines, and implicit grid lines. For reference, you can see an approximation of the different line styles in https://projects.invisionapp.com/share/3X87NEBYH#/screens/179720294_Grid for these 3 different line types.
[List of other uplifts needed for the feature/fix]: 
[Is the change risky?]: No
[Why is the change risky/not risky?]: The only changes necessary was a removal of a closePath call which is part of the canvas API and was misused in the regressing bug when called will close the path and thus closing the line that was suppose to be drawn and losing line dash styles.
[String changes made/needed]: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8971474 [details]
Bug 1452503 - Fix the implicit and explicit lines drawn on the grid highlighter.

ok, i guess.  let's take this for 60 rc, though the lack of automated tests seems like a significant shortcoming...

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/e0d995896e36
https://hg.mozilla.org/releases/mozilla-beta/rev/853522d049846ca4e6e5793db73903f6c09a329f (FIREFOX_60b_RELBRANCH)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/e0d995896e36

Comment 14

[David Olah]

I verified this issue on Nightly 61.0a1 (2018-05-02) (64-bit) and found out a few things that are unclear:

1) The up and left margins are marked by solid tracks, the bottom and right ones by dotted tracks. (Link: https://www.w3schools.com/css/tryit.asp?filename=trycss_grid_layout_named )
2) When "Display area names" is checked, all of the tracks are solid. (Link: https://www.w3schools.com/css/tryit.asp?filename=trycss_grid_layout_named )
3) The "Display are names", even if is checked doesn't show the area names. (Link: https://gridbyexample.com/examples/code/example10.html )

[Note:]
- For each number from the remarks above you can find an attachment with the print screen.

Comment 15

[David Olah]

Attachment: 1.png

Comment 16

[David Olah]

Attachment: 2.png

Comment 17

[David Olah]

Attachment: 3.png

Comment 18

[David Olah]

Gabriel, can you take a look at comment 14 and tell me what do you think?

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

1) This is expected because solid tracks are for explicit edges and you can see from the grid definition all the other columns/rows are implicitly defined. 

2) This is working as intended

3) This is working as intended. You don't see anything because there's no grid area defined like in (1).

Comment 20

[David Olah]

Thanks Gabriel for your help. I am marking this issue for Nightly 61 as verified.
I'll update the bug after I will verify also on the other platforms.

Comment 21

[David Olah]

I verified the issue on Windows 10, Ubuntu 16.04 and Mac OS X 13, Nightly 61.0a1 (2018-05-03), Firefox 60.0 and Firefox ESR 60 Build 5.
For all of the environments above, the issue is confirmed as Verified, Fixed.