1327725 - Element picker stays active in previous page when I navigate to another page

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open new tab. Focus urlbar, type "data:,a", press Enter. Focus urlbar, type "data:,b", press Enter.
2. Press Ctrl+Shift+C to open inspector
3. Hover mouse over letter "b" on the page
4. Press Alt+Left to navigate to "data:,a"
5. Press F12 to close devtools
6. Press Alt+Right to navigate to "data:,b"

AR:  Element <pre> on the page is still highlighted
ER:  No highlight at all, because I closed devtools


STR_2:  (further experiments)
1.  Open new tab. Focus urlbar, type "data:,a", press Enter. Focus urlbar, type "data:,b", press Enter
2.  Press Ctrl+Shift+C to open inspector
3.  Hover mouse over letter "b" on the page
4.  Press Alt+Left to navigate to "data:,a"
5.  Move mouse by ~5px, then hover mouse over letter "a" on the page
6.  Press Alt+Right to navigate to "data:,b"
7.  Move mouse to the center of the page
8.  Press Alt+Left to navigate to "data:,a"
9.  Move mouse by ~5px
10. Press Alt+Right to navigate to "data:,b"
11. Press F12 to close devtools
12. Press Alt+Left to navigate to "data:,a"

AR:  Steps 11, 12 - two "highlights" are displayed on the page at the same time. Devtools are closed.
ER:  Steps 6-10 - only one highlight should be displayed on the page. Steps 11,12 - no highlight.

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

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

I was able to reproduce the issue on the latest Firefox Release (51.0.1) and on the latest Nightly (54.0a1).

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

This bug is similar to other bugs we've fixed in the past – and currently, to bug 1333707 for example.
We should probably hide / destroy the box model highlighter during the `pagehide` event here as well.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

- Added `pagehide` event to hide the Box Model Highlighter

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

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

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

Comment 6

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

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

https://reviewboard.mozilla.org/r/112708/#review114432

::: devtools/server/actors/highlighters/box-model.js:105
(Diff revision 1)
>     * regionFill property: `highlighter.regionFill.margin = "red";
>     */
>    this.regionFill = {};
>  
>    this.onWillNavigate = this.onWillNavigate.bind(this);
> +  this.onPageHide = this.hide.bind(this);

Move this above this.onWillNavigate

::: devtools/server/actors/highlighters/box-model.js:263
(Diff revision 1)
>  
>    /**
>     * Destroy the nodes. Remove listeners.
>     */
>    destroy: function () {
> +    let { pageListenerTarget } = this.highlighterEnv;

Move this after 

this.highlighterEnv.off("will-navigate", this.onWillNavigate).

I also like to keep the consistent ordering of on/off of the event listeners.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112708/diff/1-2/

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112708/diff/2-3/

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

Gabriel, I need another round of review, since the code has changed; could you take a look? Thanks!

Comment 11

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

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

https://reviewboard.mozilla.org/r/112708/#review121264

::: devtools/server/actors/highlighters/box-model.js:727
(Diff revision 3)
>      if (isTopLevel) {
>        this.hide();
>      }
> +  },
> +
> +  onPageHide: function ({ target }) {

Move this above onWillNavigate

::: devtools/server/actors/highlighters/box-model.js:728
(Diff revision 3)
>        this.hide();
>      }
> +  },
> +
> +  onPageHide: function ({ target }) {
> +    // Ensure we're getting this event from the window the highlighter is handling.

We should fully explain what is happening:
If a page hide event is triggered for current window's highlighter, hide the highlighter.

::: devtools/server/actors/highlighters/css-grid.js:420
(Diff revision 3)
>      if (isTopLevel) {
>        this.hide();
>      }
>    },
>  
> +  onPageHide: function ({ target }) {

Move above onWillNavigate

::: devtools/server/actors/highlighters/css-grid.js:421
(Diff revision 3)
>        this.hide();
>      }
>    },
>  
> +  onPageHide: function ({ target }) {
> +    // Ensure we're getting this event from the window the highlighter is handling.

Rephrase the comment as above.

::: devtools/server/actors/highlighters/geometry-editor.js:392
(Diff revision 3)
>  
> -    const { type, pageX, pageY } = event;
> +    const { target, type, pageX, pageY } = event;
>  
>      switch (type) {
>        case "pagehide":
> +        // Ensure we're getting this event from the window the highlighter is handling.

Rephrase the comment.

::: devtools/server/actors/highlighters/measuring-tool.js:538
(Diff revision 3)
>          break;
>        case "scroll":
>          this.hideLabel("position");
>          break;
>        case "pagehide":
> +        // Ensure we're getting this event from the window the highlighter is handling.

Rephrase the comment.

::: devtools/server/actors/highlighters/rulers.js:204
(Diff revision 3)
>      switch (event.type) {
>        case "scroll":
>          this._onScroll(event);
>          break;
>        case "pagehide":
> +        // Ensure we're getting this event from the window the highlighter is handling.

Rephrase the comment.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112708/diff/3-4/

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8837583 [details]
Bug 1327725 - ensure the pagehide event we received are for the window's highlighter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112708/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1b91f7861ec
ensure the pagehide event we received are for the window's highlighter; r=gl

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

The code is changed since the review, since the patches made some intermittent more frequent. I shown the changes to Gabriel directly and we discussed about that.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1b91f7861ec

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

This likely fixes also few intermittents that happens in bug 1321389; we should keep on eye on that.

Comment 18

[Fahima Zulfath]

[BugDay-20170329]
The issue is no longer to be reproduced in Nightly 55.0a1

Comment 19

[Julien Cristau [:jcristau]]

Doesn't look like an uplift candidate in late beta, wontfix for 54.