1328410 - Location Bar should get focus when click the Location Bar even if identity arrow-panel had opened

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: UX regression

Reproducible: always

Steps To Reproduce:
1. Open any page (e.g. about:home )
2. Click identity icon in Location Bar so that identity arrow-panel will open.
3. Click Location Bar so that Location Bar will get foocus.

Actual Results:
The identity arrow-panel closed. 
And then Location Bar would not get focus. (i.e., Location Bar temporarily gets focus, but then loses focus immediately)

Expected Results:
The identity arrow-panel closed.
And then Location bar should get focus

Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=00195e9e7d8b954681a8c2a0784442c33e690684&tochange=11921c9f7d11e52db8da6f7dbaa9d6567227a3ad

Regressed by:11921c9f7d11	Gijs Kruitbosch — Bug 1286389 - fix focusing if nothing is focused when panel closes, r=Enn

Comment 1

[arni2033]

This is bug 1219510, but not it extends to all panels: earlier only panel w/ focusable element were affected, but after bug 1286389 all panels are affected.
I'll leave NI for Gijs to confirm info in this comment.

Comment 2

[Alice0775 White]

(In reply to arni2033 [Please stop 'improving' Firefox] from comment #1)
> This is bug 1219510, but not it extends to all panels: earlier only panel w/
> focusable element were affected, but after bug 1286389 all panels are
> affected.
> I'll leave NI for Gijs to confirm info in this comment.
> 
> *** This bug has been marked as a duplicate of bug 1219510 ***

No, this is a different scenario from bug 1219510.
No, this is a regression bug 1286389.

Comment 3

[Alice0775 White]

I suggest that bug 1286389 should be backed out from 51beta and aurora52 at least.

Comment 4

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #3)
> I suggest that bug 1286389 should be backed out from 51beta and aurora52 at
> least.

I don't think this issue is severe enough to justify backouts, also considering that backing this out will regress something very similar.

This is a popup.xml issue, so I'm moving the bug.

I also can't reproduce with the steps from comment #0 when testing on beta. I can only reproduce if the focus was somewhere other than the location bar before opening the panel (such as the web page, or the search bar), which makes some amount of sense, I guess, but is not included in the STR in comment #0.

I suspect attachment 8728495 [details] [diff] [review] from bug 1219510 will fix this, potentially with modifications to avoid regressing bug 1286389 straight away. I'll try to find time to confirm this tomorrow, but I came back from holiday today, it is now nearly night here, and I have a lot of stuff to get through. It doesn't fix the general case in bug 1219510, so this isn't a dupe, but it should fix this specific regression.

Comment 5

[Alice0775 White]

(In reply to :Gijs from comment #4)
> (In reply to Alice0775 White from comment #3)
> I don't think this issue is severe enough to justify backouts, also
> considering that backing this out will regress something very similar.

bug 1286389 is existing in Firefox50.
So, I think you can backed out from 51 and 52 without any regression from 50.

> 
> I also can't reproduce with the steps from comment #0 when testing on beta.


I can reproduce on Firefox51b10.

1. Start Firefox beta51, then about:home tab only
2. Click identity icon in Location Bar so that identity arrow-panel will open.
3. Click Location Bar so that Location Bar will get focus.

Comment 6

[Alice0775 White]

I cannot reproduce on linux.
So, this may be Windows only.

Comment 7

[Astley Chen (inactive)]

[Tracking Requested - why for this release]: regression that impact user interaction with location bar.

Alice, follow your STR, I can reproduce on macOS with 51b10 and 52 aurora. This is not Windows specific.

> So, I think you can backed out from 51 and 52 without any regression from 50.
I also agree this. Bug 1286389 is existing in 50 and it is not a found regression, also the symptom is not that serious for general users. I'll recommend to backout from 51 and spare more time to test the fix in this bug.

Comment 8

[:Gijs (he/him)]

I backed out bug 1286389 from 51 based on the comments so far.

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

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

Comment 10

[:Gijs (he/him)]

I've rebased Neil's patch on top of my changes, and I believe this works.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8823849 [details]
Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

https://reviewboard.mozilla.org/r/102332/#review102718

::: toolkit/content/widgets/popup.xml:331
(Diff revision 1)
>            } catch (e) {
>              prevFocus.focus();
>            }
>          }
>          var currentFocus = this._currentFocus;
> +        let nowFocus;

Self-nit: in terms of following the logic here, I think it'd be better if this was below the

```js
this._currentFocus = null;
this._prevFocus = null;
```
setting a bit lower down, though in practice I don't expect it'll make any difference - the binding's popuphidden code will just overwrite the values the next time it's shown.

Comment 12

[:Gijs (he/him)]

Neil, as I'm effectively submitting your patch from bug 1219510, just paging you to make sure that there wasn't an issue with that patch that I should be aware of. :-)

Comment 13

[Astley Chen (inactive)]

(In reply to :Gijs from comment #8)
> I backed out bug 1286389 from 51 based on the comments so far.

Gijs, thanks for your help.
I just found another regression(50 OK, 51b11/52.0a2 NG) which seems related to this bug as well.

STR:
1. Click 'Edit this bookmark' button.
2. On 'Page Bookmarked' dialog, expand to show all bookmark folders.
3. Double click on particular folder to gain input focus to rename folder.
4. The focus will be gone as this bug says and failed to edit the folder name.

This is another case that fails the bookmark editing function which is likely serious to users.
I'm not sure if it's in same root cause. Could you check with your patch ?

Comment 14

[:Gijs (he/him)]

(In reply to Astley Chen [:astley] UTC+8 from comment #13)
> (In reply to :Gijs from comment #8)
> > I backed out bug 1286389 from 51 based on the comments so far.
> 
> Gijs, thanks for your help.
> I just found another regression(50 OK, 51b11/52.0a2 NG) which seems related
> to this bug as well.
> 
> STR:
> 1. Click 'Edit this bookmark' button.
> 2. On 'Page Bookmarked' dialog, expand to show all bookmark folders.
> 3. Double click on particular folder to gain input focus to rename folder.
> 4. The focus will be gone as this bug says and failed to edit the folder
> name.
> 
> This is another case that fails the bookmark editing function which is
> likely serious to users.
> I'm not sure if it's in same root cause. Could you check with your patch ?

I can reproduce on 51, but this works for me on vanilla Nightly (53) without the patch on this bug (whereas this bug affects 53), and no popup panel is hidden, so I don't think it has the same cause as this bug. Please can you file a separate bug and try to find a regression / fix window? If you need help with this, please needinfo me on the new bug.

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

Tracking this new regression for 52.

Comment 16

[Neil Deakin]

I think that patch is ok. But I would add a comment to it, explaining that it is handling the case where the focus is changing while the popup is hiding.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8823849 [details]
Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

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

Comment 18

[Astley Chen (inactive)]

(In reply to :Gijs from comment #14)
> I can reproduce on 51, but this works for me on vanilla Nightly (53) without
> the patch on this bug (whereas this bug affects 53), and no popup panel is
> hidden, so I don't think it has the same cause as this bug. Please can you
> file a separate bug and try to find a regression / fix window? If you need
> help with this, please needinfo me on the new bug.

Will do. Thanks for the clarification.

Comment 19

[castiel743]

(In reply to :Gijs from comment #12)
> Neil, as I'm effectively submitting your patch from bug 1219510, just paging
> you to make sure that there wasn't an issue with that patch that I should be
> aware of. :-)

This bug is the same story as bug 1219510, and it was explained in that bug, that your patch won't fix
the issue. Everything that should've been done in this bug, is already done (backout of bug 1286389).

If you think that your patch is enough, please post in this bug an explanation, why you hesitated
in bug 1219510, keeping the issue in Firefox for nine months.
Please also post an explanation why using newly filed bug to fix old issue reported in bug 1219510.

Comment 20

[:Gijs (he/him)]

(In reply to castiel743 from comment #19)
> (In reply to :Gijs from comment #12)
> > Neil, as I'm effectively submitting your patch from bug 1219510, just paging
> > you to make sure that there wasn't an issue with that patch that I should be
> > aware of. :-)
> 
> This bug is the same story as bug 1219510,

No, it isn't, as Alice correctly pointed out in comment #2.

> the issue. Everything that should've been done in this bug, is already done
> (backout of bug 1286389).

It was only backed out of 51, not out of 52 or 53.

> If you think that your patch is enough, please post in this bug an
> explanation, why you hesitated
> in bug 1219510, keeping the issue in Firefox for nine months.

It's enough for this bug, but not enough for bug 1219510, because they're different bugs.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8823849 [details]
Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

https://reviewboard.mozilla.org/r/102332/#review103952

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a8a57f0822a
only restore focus if the user did not explicitly place focus elsewhere, r=jaws

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Astley Chen (inactive)]

(In reply to Astley Chen [:astley] UTC+8 from comment #18)
> (In reply to :Gijs from comment #14)
> > I can reproduce on 51, but this works for me on vanilla Nightly (53) without
> > the patch on this bug (whereas this bug affects 53), and no popup panel is
> > hidden, so I don't think it has the same cause as this bug. Please can you
> > file a separate bug and try to find a regression / fix window? If you need
> > help with this, please needinfo me on the new bug.
> 
> Will do. Thanks for the clarification.

As a minor note here. The symptom is NOT reproducible on a clean profile and can be recovered by re-connecting firefox sync account. The real problem is still not clear to me and hardly to move it forward given the situation like this.

Comment 25

[:Gijs (he/him)]

(In reply to Astley Chen [:astley] UTC+8 from comment #24)
> (In reply to Astley Chen [:astley] UTC+8 from comment #18)
> > (In reply to :Gijs from comment #14)
> > > I can reproduce on 51, but this works for me on vanilla Nightly (53) without
> > > the patch on this bug (whereas this bug affects 53), and no popup panel is
> > > hidden, so I don't think it has the same cause as this bug. Please can you
> > > file a separate bug and try to find a regression / fix window? If you need
> > > help with this, please needinfo me on the new bug.
> > 
> > Will do. Thanks for the clarification.
> 
> As a minor note here. The symptom is NOT reproducible on a clean profile and
> can be recovered by re-connecting firefox sync account. The real problem is
> still not clear to me and hardly to move it forward given the situation like
> this.

Sounds like this is basically bug 697649 then.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8823849 [details]
Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1286389
[User impact if declined]: focus isn't set to the location bar correctly if focus was somewhere else before opening & closing an arrow popup
[Is this code covered by automated tests?]: no :-(
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, adapted from comment #0:

> 1. Open about:home and verify its search field receives focus
> 2. Click identity icon in Location Bar so that identity arrow-panel will
> open.
> 3. Click Location Bar so that Location Bar will get focus.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very
[Why is the change risky/not risky?]:  It's a relatively small change to extant code which 3 people have now looked at and deemed reasonable, plus this is "just" aurora and all JS code...
[String changes made/needed]: no



Separately, alice, can you check this is fixed for you on nightlies with this change? Thank you!

Comment 27

[Alice0775 White]

(In reply to :Gijs from comment #26)
> Separately, alice, can you check this is fixed for you on nightlies with
> this change? Thank you!

Reproduced the problem on Nightly53.0a1(2016-01-10)
https://hg.mozilla.org/mozilla-central/rev/8f3b24109e3412b36f97277e31ad66856dc609d6
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20170110030221

And verified to fix on Nightly53.0a1(latest m-c tinderbox-build)
https://hg.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20170110031508

Comment 28

[:Gijs (he/him)]

Marking as verified per comment #27.

Comment 29

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8823849 [details]
Bug 1328410 - only restore focus if the user did not explicitly place focus elsewhere,

focus restore fix for popups, for aurora52

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f3854382eba