Closed Bug 1390561 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Application Update, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: rhelmer, Assigned: evanxd)

References

Details

(Whiteboard: [photon-preference])

Attachments

(3 files, 1 obsolete file)

Attached video Screen Recording.mov (obsolete) —
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.
Did this just start happening today? The last change to that window was in bug 1373570, but that was a little while ago.
Flags: needinfo?(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.
Flags: needinfo?(rhelmer)
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.
Flags: needinfo?(evan)
Flags: needinfo?(evan)
Whiteboard: [photon-preference][triage]
See Also: → 1373570
Assignee: nobody → evan
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(rhelmer)
Attachment #8898666 - Flags: review?(felipc)
Hi Felipe,

Could you help review the patch?

This issues is caused by Bug 1373570.

Thank you.
Attached image 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.
(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.
Attached video 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.
Attachment #8897480 - Attachment is obsolete: true
(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 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
Attachment #8898666 - Flags: review?(felipc) → review?(jaws)
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.
Attachment #8898666 - Flags: review?(jaws) → review-
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
Hi Jared,

What do you think of Comment 15?

Thank you for the previous review.
Flags: needinfo?(jaws)
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 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?
Flags: needinfo?(jaws)
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
Attachment #8898666 - Flags: review?(evan)
Attachment #8898666 - Flags: review?(evan)
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!
Attachment #8898666 - Flags: review?(jaws) → review+
Summary: using keyboard to press update in About dialog does not work → The "Update to ..." button on the About dialog doesn't get focus after the dialog is loaded
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.
Attachment #8898666 - Flags: approval-mozilla-beta?
Pushed by rchien@mozilla.com:
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
Keywords: checkin-needed
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.
Attachment #8898666 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/873f47053dd6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.