1286389 - Closing a panel doesn't place focus where it was

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Daniel Naber]

Attachment: focustest.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160706215822

Steps to reproduce:

-Install the attached WebExtension
-Place cursor in a textfield or textarea
-Click the extension's icon
-Popup will open and do nothing other than display a text
-Close popup with escape or by clicking the extension icon again
-Popup closes but the cursor doesn't get placed back to the textarea where it used to be

Why is this an issue? My extension checks text and will usually be called with a short cut (once this works). Closing the popup and losing the focus is a usability issue. Chrome places the cursor back to where it was.

Comment 1

[drhen123]

Surely if you are using the shortcut your cursor would not move from over the extension?

Comment 2

[Daniel Naber]

I cannot test with the shortcut, as it doesn't work yet (see issue 1246034). But even when the user clicks the button and then presses Escape, it seems logical to place the focus back to where it was, like Chrome does.

Comment 3

[:Gijs (he/him)]

Neil, I think this is meant to work... any ideas why it doesn't in this case?

Comment 4

[Neil Deakin]

Should work unless norestorefocus is used. You can look what is happening the popup's popuphidden handler:

  https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.xml#318

Comment 5

[Neil Deakin]

A brief test shows that the focus is being cleared at some point. We probably want to change the popuphidden handler to allow restoring focus if 'currentFocus' is null.

Comment 6

[Kris Maglione [:kmag]]

Is the previous focus correct, even with currentFocus being null? I'd expect the original focus to be cleared when the toolbar button was clicked, before the popup opened and tried to save it.

Comment 7

[Neil Deakin]

No, the issue is that something is clearing the focus while the popup is open and the focus-restoring-code assumes that if something changes the focus while it was open that the popup did this intentionally and doesn't want to have focus restored.

But having the focus cleared seems like it should be an exception.

Comment 8

[:Gijs (he/him)]

Attachment: fix focusing if nothing is focused when panel closes,

MozReview-Commit-ID: DzNnQZjuXK2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3696a22ff7a0
fix focusing if nothing is focused when panel closes, r=Enn

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3696a22ff7a0

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out for likely causing frequent linux debug dt2 failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32220914&repo=mozilla-inbound#L4785

Comment 12

[:Gijs (he/him)]

So the test output looks like:

 10:00:31     INFO -  29 INFO checking window state
 10:00:31     INFO -  30 INFO Entering test bound
 10:00:31     INFO -  31 INFO Adding a new tab with URL: data:text/html;charset=utf-8,%0A%20%20%3Cstyle%20type%3D%22text%2Fcss%22%3E%0A%20%20%20%20.matches%20%7B%0A%20%20%20%20%20%20color%3A%20%23F00%3B%0A%20%20%20%20%7D%0A%20%20%3C%2Fstyle%3E%0A%20%20%3Cspan%20id%3D%22matches%22%20class%3D%22matches%22%3ESome%20styled%20text%3C%2Fspan%3E%0A
 10:00:31     INFO -  32 INFO Waiting for event: 'load' on [object XULElement].
 10:00:31     INFO -  33 INFO Got event: 'load' on [object XULElement].
 10:00:31     INFO -  34 INFO Tab added and finished loading
 10:00:31     INFO -  35 INFO Opening the inspector
 10:00:31     INFO -  36 INFO Opening the toolbox
 10:00:31     INFO -  37 INFO Toolbox opened and focused
 10:00:31     INFO -  38 INFO Need to wait for the inspector to update
 10:00:31     INFO -  39 INFO Waiting for actor features to be detected
 10:00:31     INFO -  40 INFO Selecting the computedview sidebar
 10:00:31     INFO -  41 INFO Selecting the node for '#matches'
 10:00:31     INFO -  42 INFO checking "Browser styles" checkbox
 10:00:31     INFO -  43 INFO setting filter text to "color"
 10:00:31     INFO -  44 INFO TEST-PASS | devtools/client/inspector/computed/test/browser_computed_search-filter.js | Search field isn't focused -
 10:00:31     INFO -  45 INFO TEST-PASS | devtools/client/inspector/computed/test/browser_computed_search-filter.js | Search field is focused -
10:00:31 INFO - 46 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/computed/test/browser_computed_search-filter.js | Test timed out -

 Based on the screenshot, the tools aren't even fully open, though: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/fx-team/sha512/739a15b604a71e72441e238a1b43552d84d704459acac154a9ad0232f53d349e862fc9692d101a5d4202298cb6f1e9561fb4b784724515fca2ec8a5535d1c7c1

Gabriel/Brian, any idea what's going on and why this change would break that test?

Comment 13

[:Gijs (he/him)]

FWIW, the inbound breakage from comment #11 does have a screenshot showing the tools are open.

Also not sure why/how panels are involved in this test.

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ee1973d27f13
Backed out changeset 3696a22ff7a0 for frequent dt2 linux debug failures

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #13)
> FWIW, the inbound breakage from comment #11 does have a screenshot showing
> the tools are open.
> 
> Also not sure why/how panels are involved in this test.

Panels shouldn't be involved at all, that's testing the filter that happens when you type a css property name in the Computed View of the inspector.  That screenshot looks clearly wrong though, since the filter box has "background-color" and it should be "color" in the text field: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-inbound/sha512/25b5a28e5917fd0dd5c14f7b41db5ad62eaf802185cb1bab04a415e56a10fb42e6a71183be485bb20a3866053d61c3b437e7c291af0629ec9ad684cd7da6ae85.  Gabe

Comment 16

[Brian Grinstead [:bgrins]]

.. Gabe, have you seen that before?

Comment 17

[Brian Grinstead [:bgrins]]

FWIW, I applied the patch locally and unfortunately couldn't get the failure with --run-until-failure

Comment 18

[Brian Grinstead [:bgrins]]

Went ahead with another try push just to be sure that it's reproducing on latest fx-team: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3208f30182b1.

Comment 19

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Went ahead with another try push just to be sure that it's reproducing on
> latest fx-team:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3208f30182b1.

Here it doesn't happen on linux debug but happens on linux64 debug instead. :-\

I don't really have time to look into this in more detail, and I'm on PTO the next 2 weeks, so unassigning for now. Hopefully someone else can push this over the line.

Comment 20

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #13)
> > FWIW, the inbound breakage from comment #11 does have a screenshot showing
> > the tools are open.
> > 
> > Also not sure why/how panels are involved in this test.
> 
> Panels shouldn't be involved at all, that's testing the filter that happens
> when you type a css property name in the Computed View of the inspector. 

Is there not some kind of first-run / explanation / UITour / "use this syntax" panel that only shows once or something that could potentially explain this? Like there is for the filter box in the debugger?

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #20)
> (In reply to Brian Grinstead [:bgrins] from comment #15)
> > (In reply to :Gijs Kruitbosch from comment #13)
> > > FWIW, the inbound breakage from comment #11 does have a screenshot showing
> > > the tools are open.
> > > 
> > > Also not sure why/how panels are involved in this test.
> > 
> > Panels shouldn't be involved at all, that's testing the filter that happens
> > when you type a css property name in the Computed View of the inspector. 
> 
> Is there not some kind of first-run / explanation / UITour / "use this
> syntax" panel that only shows once or something that could potentially
> explain this? Like there is for the filter box in the debugger?

Not that I'm aware of.  There is a panel in beta builds that shows up when the toolbox is first opened to promote dev edition, but I don't know of anything in the inspector specifically, or in a non branded build.

Comment 22

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

Sorry, no idea what is causing this issue.

Comment 23

[:Gijs (he/him)]

Let's just try again, maybe it's gone away... if not, we can go from there. I don't see anything locally, at least.

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d938fa8b605d

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11921c9f7d11
fix focusing if nothing is focused when panel closes, r=Enn

Comment 25

[:Gijs (he/him)]

Retriggers don't seem to indicate this is causing frequent unknown orange, so I relanded.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11921c9f7d11

Comment 27

[:Gijs (he/him)]

This didn't get uplifted so is only fixed on 51.

Comment 28

[:Gijs (he/him)]

Backed out of 51 for causing bug 1328410.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1672ba92485f35fad11519b18fb9f0136aa09c4d

Comment 29

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336