Closed Bug 1327725 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 verified)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: arni2033, Assigned: zer0)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

>>>   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.
No longer blocks: 1277113
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).
Component: Untriaged → Developer Tools: Inspector
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.
Assignee: nobody → zer0
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Note: this patch is built on top of bug 1312103's patch.
See Also: → 1333707
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.
Attachment #8837583 - Flags: review?(gl) → review+
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!
Attachment #8837583 - Flags: review+ → review?(gl)
Duplicate of this bug: 1345292
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.
Attachment #8837583 - Flags: review?(gl) → review+
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1b91f7861ec
ensure the pagehide event we received are for the window's highlighter; r=gl
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.
https://hg.mozilla.org/mozilla-central/rev/b1b91f7861ec
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
This likely fixes also few intermittents that happens in bug 1321389; we should keep on eye on that.
See Also: → 1321389
[BugDay-20170329]
The issue is no longer to be reproduced in Nightly 55.0a1
Doesn't look like an uplift candidate in late beta, wontfix for 54.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.