Closed Bug 1286389 Opened 3 years ago Closed 3 years ago

Closing a panel doesn't place focus where it was

Categories

(Core :: XUL, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: naber, Assigned: Gijs)

References

Details

Attachments

(2 files)

Attached file 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.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Surely if you are using the shortcut your cursor would not move from over the extension?
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.
Component: WebExtensions → General
Product: Toolkit → Firefox
Summary: WebExtension: closing popup doesn't place focus where it was → Closing a panel doesn't place focus where it was
Neil, I think this is meant to work... any ideas why it doesn't in this case?
Component: General → XP Toolkit/Widgets: XUL
Flags: needinfo?(enndeakin)
Product: Firefox → Core
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
Flags: needinfo?(enndeakin)
Flags: needinfo?(gijskruitbosch+bugs)
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.
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.
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.
MozReview-Commit-ID: DzNnQZjuXK2
Attachment #8772416 - Flags: review?(enndeakin)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8772416 - Flags: review?(enndeakin) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/3696a22ff7a0
fix focusing if nothing is focused when panel closes, r=Enn
https://hg.mozilla.org/mozilla-central/rev/3696a22ff7a0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Status: RESOLVED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
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?
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: needinfo?(gl)
Flags: needinfo?(bgrinstead)
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
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.
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/ee1973d27f13
Backed out changeset 3696a22ff7a0 for frequent dt2 linux debug failures
(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
Flags: needinfo?(bgrinstead)
.. Gabe, have you seen that before?
FWIW, I applied the patch locally and unfortunately couldn't get the failure with --run-until-failure
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.
(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.
Assignee: gijskruitbosch+bugs → nobody
Status: REOPENED → NEW
(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?
(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.
Flags: needinfo?(gl)
Sorry, no idea what is causing this issue.
Flags: needinfo?(gijskruitbosch+bugs)
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
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11921c9f7d11
fix focusing if nothing is focused when panel closes, r=Enn
Retriggers don't seem to indicate this is causing frequent unknown orange, so I relanded.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/11921c9f7d11
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
This didn't get uplifted so is only fixed on 51.
Depends on: 1328410
Backed out of 51 for causing bug 1328410.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1672ba92485f35fad11519b18fb9f0136aa09c4d
Target Milestone: mozilla50 → mozilla52
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.