1390561 - The "Update to ..." button on the About dialog doesn't get focus after the dialog is loaded

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Robert Helmer [:rhelmer]]

Attachment: Screen Recording.mov

If I open the "About Nightly" dialog on macOS, I am able to press space or return to trigger the button which seems to visually activate the button, but nothing happens unless I actually click on it with the mouse.

Comment 1

[Molly Howell (she/her) (mostly inactive)]

Did this just start happening today? The last change to that window was in bug 1373570, but that was a little while ago.

Comment 2

[Robert Helmer [:rhelmer]]

(In reply to Matt Howell [:mhowell] from comment #1)
> Did this just start happening today? The last change to that window was in
> bug 1373570, but that was a little while ago.

No I think it's been happening for a while, I just only use this dialog sporadically and I don't consistently use just the keyboard or the mouse so it took me a while to notice.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Evan, could you take a look at this? The only recent change I can find that's directly relevant to the about dialog button is your patch for bug 1373570. Thanks.

Comment 4

[Evan Tseng [:evanxd]]

Hi Matt,

Let me investigate this. Honestly, I'm not sure if Bug 1373570 cause this issue since the only thing I did in the bug is making the button have focus.

Comment 5

[Evan Tseng [:evanxd]]

Hi Robert,

I have two questions about this bug.

1. Could you tell me that the "Update to 57.0a1" button in the attachment 8897480 [details] video has focus or not? Looks like it doesn't have the focus, right? I just want to ensure is it just a focus issue or we need to dig deeper?
2. How do you reproduce the bug since my Nightly build is already the latest version so there is no "Update to 57.0a1" button in my about dialog. Do you know we have any way to reproduce the issue or show the button?

And thank you for filing the issue.

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Evan Tseng [:evanxd] from comment #5)
> Hi Robert,
> 
> I have two questions about this bug.
> 
> 1. Could you tell me that the "Update to 57.0a1" button in the attachment
> 8897480 [details] video has focus or not? Looks like it doesn't have the
> focus, right? I just want to ensure is it just a focus issue or we need to
> dig deeper?

The window does have focus, and the button appears to get auto-focus. In the video I am tapping spacebar several times (you can see the button disappear briefly each time), and then finally mouse clicking which works.

> 2. How do you reproduce the bug since my Nightly build is already the latest
> version so there is no "Update to 57.0a1" button in my about dialog. Do you
> know we have any way to reproduce the issue or show the button?

Right now I am on macOS 57.0a1 (2017-08-16) and I can repro it right now, but this is a good point :) I suspect that pointing the app.update.url pref at a valid XML update file will repro the issue but I haven't tried this, folks that work on this code might have an easier way.

> And thank you for filing the issue.

Comment 7

[Evan Tseng [:evanxd]]

Attachment: Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

* The update component is now in General category in Preference page.
* Preference page now focuses the search input when it is loaded.

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

Comment 8

[Evan Tseng [:evanxd]]

Hi Felipe,

Could you help review the patch?

This issues is caused by Bug 1373570.

Thank you.

Comment 9

[Evan Tseng [:evanxd]]

Attachment: button-focused.png

(In reply to Robert Helmer [:rhelmer] from comment #6)
> (In reply to Evan Tseng [:evanxd] from comment #5)
> The window does have focus, and the button appears to get auto-focus. In the
> video I am tapping spacebar several times (you can see the button disappear
> briefly each time), and then finally mouse clicking which works.

Hi Robert

In the attachment 8897480 [details] video, the button doesn't have the focus. If the button has the focus it will have a bright border, like the attachment.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Evan Tseng [:evanxd] from comment #9)
> Created attachment 8898672 [details]
> button-focused.png
> 
> (In reply to Robert Helmer [:rhelmer] from comment #6)
> > (In reply to Evan Tseng [:evanxd] from comment #5)
> > The window does have focus, and the button appears to get auto-focus. In the
> > video I am tapping spacebar several times (you can see the button disappear
> > briefly each time), and then finally mouse clicking which works.
> 
> Hi Robert
> 
> In the attachment 8897480 [details] video, the button doesn't have the
> focus. If the button has the focus it will have a bright border, like the
> attachment.

I think this might be an artifact of the video capture software - I definitely see the blue border when I repro this locally. I'll see if I can get a clearer screencap.

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: Screen Recording.mov

Hopefully this is clearer - you can see that the "Check for update" button is auto-focused initially and works fine selecting w/ spacebar. The "Update to 57.0a1" button does not have a focus highlight around it, and does seem to do something in reaction to pressing spacebar (you can see the button disappear briefly) but the button click isn't registered until I click w/ the mouse.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to Robert Helmer [:rhelmer] from comment #11)
> Created attachment 8898862 [details]
> Screen Recording.mov
> 
> Hopefully this is clearer - you can see that the "Check for update" button
> is auto-focused initially and works fine selecting w/ spacebar. The "Update
> to 57.0a1" button does not have a focus highlight around it, and does seem
> to do something in reaction to pressing spacebar (you can see the button
> disappear briefly) but the button click isn't registered until I click w/
> the mouse.

It looks like that the spacebar presses are still going to the "Check for updates" button, even though it's invisible

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review175402

I should redirect this to Jared

Comment 14

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

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review175478

::: browser/base/content/aboutDialog-appUpdater.js:190
(Diff revision 1)
> +      this.updateDeck.selectedPanel = panel;
> +      if (this.options.buttonAutoFocus &&

I don't actually see how this code is much different than what we were doing before.

Is the root problem here that the selectedPanel could be wrong in the old code, and this new code path will always set it correctly?

If that's the case, this seems like the wrong way to fix it. We shouldn't push this code in to selectPanel() since it isn't actually about selecting the panel.

Comment 15

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review175784

::: browser/base/content/aboutDialog-appUpdater.js:190
(Diff revision 1)
> +      this.updateDeck.selectedPanel = panel;
> +      if (this.options.buttonAutoFocus &&

No, the root cause of this bug is a timing issue caused by Bug 1373570's patch[1]. In the patch[1], we might do `button.focus();` too early since correct panel of `updateDeck` will be slected and shown after update check is completed[2].

So the correct timing to do the focus is after the  `onCheckComplete` method is triggered and selects the panel. And the simple way to do this is moving the focus logic back to the `selectPanel` method and adding a flag to enable/disable the focus.

What do you think of this approach, or you have any other better idea?

[1]: https://reviewboard.mozilla.org/r/153078/diff/2#index_header
[2]: http://searchfox.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#255

Comment 16

[Evan Tseng [:evanxd]]

Hi Jared,

What do you think of Comment 15?

Thank you for the previous review.

Comment 17

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review175786

::: browser/base/content/aboutDialog-appUpdater.js:190
(Diff revision 1)
> +      this.updateDeck.selectedPanel = panel;
> +      if (this.options.buttonAutoFocus &&

The below focus logic is in the `selectPanel` method before Bug 1365133[1] is landed. So what we do in this patch is moving the focus logic back to the method and adding the flag to enable/disable the automatic button focus to avoid the timing issue.

```
if (this.options.buttonAutoFocus &&
    (!document.commandDispatcher.focusedElement || // don't steal the focus
     document.commandDispatcher.focusedElement.localName == "button")) { // except from the other buttons
  button.focus();
}
```

[1]: https://reviewboard.mozilla.org/r/150718/diff/33#index_header

Comment 18

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

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review177060

::: browser/base/content/aboutDialog-appUpdater.js:190
(Diff revision 1)
> +      this.updateDeck.selectedPanel = panel;
> +      if (this.options.buttonAutoFocus &&

Can we just put this code in onCheckComplete after the call to selectPanel?

Comment 19

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review177746

::: browser/base/content/aboutDialog-appUpdater.js:190
(Diff revision 1)
> +      this.updateDeck.selectedPanel = panel;
> +      if (this.options.buttonAutoFocus &&

Hi Jared,

I think we could not do that because it could not handle the case we want to focus the button before onCheckComplete event or other cases without onCheckComplete event. For example, how could we handle the `this.selectPanel("apply");` call in the `appUpdater` constructor [1].

So I think we might still keep the code in the original place before Bug 1365133 is landed.

What do you think of this? Or I misunderstood something?

[1]: http://searchfox.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#68

Comment 20

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

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

Comment 21

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

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

https://reviewboard.mozilla.org/r/170046/#review179076

Okay, this is fine. Thanks!

Comment 22

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

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

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170046/diff/3-4/

Comment 24

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170046/diff/4-5/

Comment 25

[Evan Tseng [:evanxd]]

The try[1] is good. Let's land the patch.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08c694f28c26&selectedJob=126964579

Comment 26

[Evan Tseng [:evanxd]]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1373570
[User impact if declined]: Users cannot update to new version of Firefox with pressing space (or enter) key. Users need to use mouse to click the button to update. Because the button loses the focus in current build.
[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.
STR:
1. Open About dialog.
2. Open "Browser Toolbox" (DevTools) and switch to "chrome://browser/content/aboutDialog.xul"
3. Open the DevTools console and input the `Object.defineProperties(gAppUpdater, { "update": { value: { displayVersion: "57.0a1", "buildID": "20170830000000" } } }); gAppUpdater.selectPanel('downloadAndInstall');` script.

Excepted:
The button has the focus, like the http://imgur.com/a/I58ji screenshot.

Before this bug is landed:
The button doesn't have the focus, like the http://imgur.com/a/43wal screenshot.

[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Small.
[Why is the change risky/not risky?]: Because we only move a if condition (which focus the button) to correct place and it is a 14 lines small change.
[String changes made/needed]: None.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/873f47053dd6
Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page. r=jaws

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8898666 [details]
Bug 1390561 - Add a new param "buttonAutoFocus" for appUpdater object to focus the button automatically because we need auto focus in about dialog but do not need it in preferences page.

Fix for a regression from 56, let's uplift this for beta 7.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2badd2f31dc6

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/873f47053dd6

Comment 31

[Hani Yacoub, Desktop QA]

Build ID: 20170912013600
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.