Closed Bug 1376774 Opened 7 years ago Closed 7 years ago

div.content.grid-content checkbox is displayed unchecked in Inspector but appears as enabled on the webpage after refresh

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox54 unaffected, firefox55 wontfix, firefox56 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox54 --- unaffected
firefox55 --- wontfix
firefox56 --- verified

People

(Reporter: cgeorgiu, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached image screencast - issue.gif
[Affected versions]:
- latest Nightly 56.0a1
- Beta 55.0b5 (20170626165718)

[Affected platforms]:
- Windows 10 x64
- macOS 10.12.5
- Ubuntu 16.04 x64 LTS

[Steps to reproduce]:
1. Start Firefox.
2. Go to: https://www.mozilla.org/en-US/developer/css-grid/?utm_source=gridtooltip&utm_medium=devtools&utm_campaign=cssgrid_layout
3. Open Inspect Element (F12) and go to the Layout tab.
4. Check "div.content.grid-content".
5. Refresh the webpage. (I've noticed that sometimes it requires more than 1 refresh to reproduce this)

[Expected result]:
a) The CSS Grid checkbox stays enabled in Inspector after refresh or
b) The CSS Grid checkbox is disabled in the Inspector and Grids are not displayed anymore on the page. (after refresh)

[Actual result]:
- The CSS Grid appears enabled on the test page but "div.content.grid-content" checkbox is unchecked in Layout tab.

[Regression range]:
- If the right expected result is b) then it seems to be a regression, as I cannot reproduce it on 53.0a1 when this feature was first implemented. In that case I will follow up with a regression range.

[Additional notes]:
- Please note that with other Overlay Grids, e.g.: div.content.grid-content, div.content.grid-content this issue is not reproducible
- .gif attached showing this issue
The expected result should be a) since we have fixed bug 1342310 that makes highlighted grids re-highlight themselves after a page refresh.
This seems like a somewhat random init bug. When it happens, if you switch to the rule-view and then back to the layout-view, then the checkbox will become checked again.
This is most likely a race condition between the initialization of the grid list and of the state restoring done in the highlighter-overlay. The grid list should wait until the state has been restored, so that it shows the right checkbox being checked.
Patrick, are you considering this a blocker to ship the feature? Or is it rare enough that it shouldn't?
Flags: needinfo?(pbrosset)
I don't think this should block shipping the feature. But I'll let Gabriel take the final decision on this.
Flags: needinfo?(pbrosset) → needinfo?(gl)
Assignee: nobody → gl
Status: NEW → ASSIGNED
Flags: needinfo?(gl)
Flags: qe-verify+
Comment on attachment 8889605 [details]
Bug 1376774 - Restore highlighter states on markup loaded prior to emitting the 'new-root' event.

https://reviewboard.mozilla.org/r/160626/#review166450

Doesn't completely fix the issue for me. On http://labs.jensimmons.com/2016/examples/grid-content-1.html, after highlighting the grid and reloading the page I get the following stack trace:

console.error: 
  Message: TypeError: can't access dead object
  Stack:
    renderGridGap@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:1527:26
renderLines@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:1316:9
renderFragment@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:1131:5
_update@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/css-grid.js:855:7
update@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:240:5
_startRefreshLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:276:5
show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:110:5
show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters.js:505:12
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1799:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7

And the grid is not displayed after the reload. Subsequent reloads display the grid without any issue. 
Now that we are not waiting for for new root, maybe we are trying to redisplay the grid slightly too soon?

The issue here is with the gridGapPattern, that is normally cleared on navigate, in the css grid highlighter.
Calling this._clearCache() in onWillNavigate seems to fix the issue, but this requires more testing. Maybe check with zer0 to see what he thinks about that.

::: devtools/client/inspector/inspector.js:808
(Diff revision 1)
> +
> +    this.markup.expandNode(this.selection.nodeFront);
> +
> +    // Restore the highlighter states prior to emitting "new-root".
> +    yield this.highlighters.restoreGridState();
> +    yield this.highlighters.restoreShapeState();

We could yield Promise.all([restoreGrid(), restoreShape()]) here.
Attachment #8889605 - Flags: review?(jdescottes)
Attached patch 1376774.patchSplinter Review
Moving clearCache to will-navigate should be more than fine.
Attachment #8889605 - Attachment is obsolete: true
Attachment #8890393 - Flags: review?(jdescottes)
Comment on attachment 8890393 [details] [diff] [review]
1376774.patch

Review of attachment 8890393 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, r+ with a green try.
Attachment #8890393 - Flags: review?(jdescottes) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4bd3b77a17
Restore the highlighter states on markup loaded prior to emitting the "new-root" event. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/aa4bd3b77a17
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Verified that using latest Nightly across platforms(macOS 10.12.5, Windows 7 32bit and Ubuntu 16.04 64bit), I no longer encounter this issue. I'll go ahead and mark as wontfix on Fx55 because you have to force enable the feature and the plan is to ride 56 so I doubt it's worth investing time in pushing to 55. Please feel free to correct me if I'm wrong.
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.