Grid stuck on page when using back button (actually there are two grids, one stuck and one active even though grid tool is not active from Rules) - Visit http://labs.jensimmons.com/examples/grid-content-1.html - Open Inspector and click on Main - From Rules activate the CSS Grid tool - On navigation bar visit another website - Hit back button after the above page is loaded - Open Inspector and check for the CSS Grid tool Screencast showing the issue: https://dl.dropboxusercontent.com/u/109148197/CSS%20Grid%20Inspector/Grid%20stuck%20when%20using%20back%20button.mov

We got similar issues on other highlighters in the past; we ended up to just turned off the highlighter when the user navigates. We can probably doing the same thing here.

Here the relevant code: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/rulers.js#203-205

Note: this patch is built on top of bug 1312103's patch.

Comment on attachment 8837549 [details] Bug 1333707 - Grid stuck on page when using back button; https://reviewboard.mozilla.org/r/112690/#review114426 ::: devtools/server/actors/highlighters/css-grid.js:105 (Diff revision 1) > this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv, > this._buildMarkup.bind(this)); > > this.onNavigate = this.onNavigate.bind(this); > this.onWillNavigate = this.onWillNavigate.bind(this); > + this.onPageHide = this.hide.bind(this); I typically like to keep things alphabetically order. So, can you move this.onPageHide above this.onWillNavigate.

We also should add a test for this using the grid highlighter. We do severely lack unit testing when it comes to page navigations and checking the states whether it is highlighters-overlay state or the ui state.

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #6) > We also should add a test for this using the grid highlighter. I'm not sure how we could test this, since we cannot inspect the anonymous content – basically the anonymous content is living in the BFCache when we go back, so we cannot access to it by inspector neither JS. What we could try, could be adding a event listener for `pagehide` in the test, and check that the highlighter is actually hidden/destroyed when this event is fired – since we had this after the highlighter creation, it should be called after. That's the first thing comes to my mind, but I'm open to suggestions.

This would really just be testing if the highlighter is still shown and checking that gridHighlighterShown in highlighters-overlay is null after going back to the previous page and then back to page where you turn on the grid. highlighters-overlay.js is where we keep track of the states of the highlighters since those states are also used in the ui to toggle if the grid button is active or not. This test is pretty close /inspector/rules/test/browser_rules_grid-highlighter-on-navigate.js

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #9) > This would really just be testing if the highlighter is still shown and > checking that gridHighlighterShown in highlighters-overlay is null after > going back to the previous page and then back to page where you turn on the > grid. highlighters-overlay.js is where we keep track of the states of the > highlighters since those states are also used in the ui to toggle if the > grid button is active or not. Sorry if I wasn't clear, but as said the anonymous content (the grid highlighter) at that point would be part of the bfcache per se, so such test even without patch, it would pass. But visually, the grid will be still there. To be clear, we could have a test like this (based on the one above): info("Toggling ON the CSS grid highlighter from the rule-view."); let onHighlighterShown = highlighters.once("grid-highlighter-shown"); gridToggle.click(); yield onHighlighterShown; ok(highlighters.gridHighlighterShown, "CSS grid highlighter is shown."); yield navigateTo(inspector, TEST_URI_2); ok(!highlighters.gridHighlighterShown, "CSS grid highlighter is hidden."); // pseudo code goBack(); yield waitForPageShow(); ok(!highlighters.gridHighlighterShown, "CSS grid highlighter is hidden."); This test will pass no matter what (with and without this patch), for "us" the highlighter is hidden, but since the HTML in the anonymous content is "cached" it will be still visible for the user.

I wouldn't stop landing this bug – if we want to have a test coverage for that, we could file another one and discuss about possible approaches there –; my suggestion is adding in the meantime the qe-verify flag here (something I was already thinking after the try build was over). What do you think?

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11) > I wouldn't stop landing this bug – if we want to have a test coverage for > that, we could file another one and discuss about possible approaches there > –; my suggestion is adding in the meantime the qe-verify flag here > (something I was already thinking after the try build was over). > What do you think? Sure, that sounds good. Unfortunately, this is one of those edge cases that either need some documentation somewhere to say that we need to hide the highlighter on "page-hide" so that anyone creating a new highlighter knows that this is one of the edge cases that needs to be handle. The unit test would've only prevented regressions, but I think ideally we need a way to pass on this knowledge.

Pushed by mferretti@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e15e6a49b1a Grid stuck on page when using back button; r=gl

Verified fixed using the latest Nightly 54.0a1 (2017-02-20) on Ubuntu 16.04, Mac OS X 10.11 and Windows 10 x64.