835722 - Infobar reappears even when not needed.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

Two situations when the infobar reappears even if it is not required to.

1) Open inspector and lock and node (or even not)
2) Switch to some other tool, lets say Web Console
3) Infobar disappears (the Highlighter)
3) Refresh the page.
4) Infobar reappears.

1) Open inspector and select any node.
2) Select a non node element, like a comment or script or head using the markup view.
3) Infobar disappears.
4) Move your mouse in the Rules side bar, and now move outside of it.
5) Infobar reappears, with the previously selected node selected.

In both the cases infobar should not reappear.

Comment 2

[Rob Campbell [:rc] (:robcee)]

any chance of a fix for this before the merge on Feb 18th? This is an annoying bug.

Comment 3

[Girish Sharma [:Optimizer]]

I can take a look at this on Friday (if no one has fixed it yet).

Comment 4

[Paul Rouget [:paul]]

The short version: instead of hiding the highlighter, let's destroy it when the inspector is not selected.

The main issue here is that we have 'hide' and 'show' functions, and we call 'show' a little too easily.

What I would do, for now, is to just destroy the highlighter when the inspector is not selected. In the future, I'd like to move the highlighter at the toolbox level (so other tools could use it) and we will then have a better control on how to show/hide it.

Comment 5

[Girish Sharma [:Optimizer]]

How about in the highlighter, or inspector, maintain a state which will tell whether to show the highlighter or not. So even if the user calls show(), infobar will not appear if the state says hide.

Comment 6

[Paul Rouget [:paul]]

Both approach would work I guess. Killing is safer than hiding I think. I'd say it's up to the one who will fix this bug :)

Comment 7

[Girish Sharma [:Optimizer]]

It was bound to happen!

Comment 8

[Paul Rouget [:paul]]

I think so :)

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

Fix.

Comment 10

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #9)
> Created attachment 714460 [details] [diff] [review]
> patch v1.0
> 
> Fix.

No test?

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 714460 [details] [diff] [review]
patch v1.0

What about renaming "forceHidden" to "disabled"?

Comment 12

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #10)
> (In reply to Girish Sharma [:Optimizer] from comment #9)
> > Created attachment 714460 [details] [diff] [review]
> > patch v1.0
> > 
> > Fix.
> 
> No test?

on their way.

(In reply to Paul Rouget [:paul] from comment #11)
> Comment on attachment 714460 [details] [diff] [review]
> patch v1.0
> 
> What about renaming "forceHidden" to "disabled"?

if that makes more sense. Okay.

Comment 13

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1 with tests.

Added tests for the two cases that this bug fixes.

Pushed to try at : https://tbpl.mozilla.org/?tree=Try&rev=089f56ec7991

Comment 14

[Girish Sharma [:Optimizer]]

try is green. All I need is an r+ :)

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 714772 [details] [diff] [review]
patch v1.1 with tests.

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

Will try to land tomorrow. Thanks Optimizer.

Comment 16

[Joe Walker [:jwalker]]

Gak - this is in need of a rebase:

patching file browser/devtools/inspector/InspectorPanel.jsm
Hunk #1 FAILED at 237
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/inspector/InspectorPanel.jsm.rej
patching file browser/devtools/inspector/test/Makefile.in
Hunk #1 FAILED at 32
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/inspector/test/Makefile.in.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh infobar-835722.patch


(At least with my patch queue)
$ hg qser
destroyed-840156.patch: Bug 840156 - Inspector doesn't gracefully handle onDOMReady event that fires after...
targeturl-734664.patch: Bug 734664 - Devtools toolbox should display the actual target url when detached. ...
typo391-839890.patch: Bug 839890 - Fix for typo at line 391 in gDevTools.jsm; r=paul

Comment 17

[Girish Sharma [:Optimizer]]

Attachment: rebased

Rebased as per the series.

Comment 18

[Joe Walker [:jwalker]]

https://hg.mozilla.org/integration/fx-team/rev/4d45bcf676f9

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4d45bcf676f9

Comment 20

[Ed Morley [:emorley]]

Backed this out for now, due to the frequency of bug 843677:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5c4d4db2a0

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #20)
> Backed this out for now, due to the frequency of bug 843677:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5c4d4db2a0

Why wasn't just the test disabled ?

Comment 22

[Ed Morley [:emorley]]

(In reply to Girish Sharma [:Optimizer] from comment #21)
> Why wasn't just the test disabled ?

Because it's the decision of the reviewer whether something can land without [enabled] tests, also in many cases disabled tests stay that way forever.

Comment 23

[Girish Sharma [:Optimizer]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #22)
> (In reply to Girish Sharma [:Optimizer] from comment #21)
> > Why wasn't just the test disabled ?
> 
> Because it's the decision of the reviewer whether something can land without
> [enabled] tests, also in many cases disabled tests stay that way forever.

Okay, anyways it was just next on my list and since this landed in Nightly only, I still have time to get this relanded.

Comment 24

[Paul Rouget [:paul]]

Any update on this?

Comment 25

[Girish Sharma [:Optimizer]]

I have a patch, but I want to test it locally first. From past 2 days, I have been struggling with Clobbers :|
Even clobbers are failing for me (without any patch, just the latest fx-team).

So once my firefox is built, I will test and submit the patch.

Comment 26

[Girish Sharma [:Optimizer]]

Attachment: tests pass.

Making sure that the event is dispatched and passed on to the window.

try is green at :
https://tbpl.mozilla.org/?tree=Try&rev=667ee00220f2 b-c only

https://tbpl.mozilla.org/?tree=Try&rev=33e4c79c904c complete try.

All failures are unrelated.

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 723136 [details] [diff] [review]
tests pass.

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

Looks good to me, r+.

Comment 28

[Girish Sharma [:Optimizer]]

I hope this time it does not fail me !

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/6b087f0ef2cb

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6b087f0ef2cb