Closed Bug 1452503 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

60 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 verified, firefox59 unaffected, firefox60 verified, firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox59 --- unaffected
firefox60 --- verified
firefox61 --- verified

People

(Reporter: kev.landry, Assigned: gl)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files)

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.
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?
Blocks: 1432036
Flags: needinfo?(mashuoyi111)
Keywords: regression
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
Gabriel, you reviewed the regressing patch, could you help?
Flags: needinfo?(gl)
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.
Priority: -- → P2
Assignee: nobody → gl
Status: NEW → ASSIGNED
Flags: needinfo?(gl)
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.
Attachment #8971474 - Flags: review?(pbrosset) → review?(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.
Attachment #8971474 - Flags: review?(jdescottes) → review+
Pushed by gabriel.luong@gmail.com:
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
Flags: needinfo?(mashuoyi111)
Needinfo myself to uplift.
Flags: needinfo?(gl)
Flags: needinfo?(gl)
Summary: Devtool grid explicite/implicite tracks → Implicit and Explicit Grid Lines are drawn with a close path and are no longer showing their dotted pattern
Blocks: dt-grid
https://hg.mozilla.org/mozilla-central/rev/74a88bd1be71
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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
Attachment #8971474 - Flags: approval-mozilla-beta?
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...
Attachment #8971474 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Flags: qe-verify+
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.
Attached image 1.png
Attached image 2.png
Attached image 3.png
Gabriel, can you take a look at comment 14 and tell me what do you think?
Flags: needinfo?(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).
Flags: needinfo?(gl)
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.