939192 - Defect - error in sync settings: "ReferenceError: AboutFlyout is not defined"

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Flyouts, defect, P2)

Description

[Jim Mathies [:jimm]]

[JavaScript Error: "ReferenceError: AboutFlyout is not defined" {file: "chrome://browser/content/flyoutpanels/AboutFlyoutPanel.js" line: 86}]

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 833094 [details] [diff] [review]
fix

Also added a unload to shut down update checking rather than grouping that with the load call.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 833094 [details] [diff] [review]
fix

Review of attachment 833094 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/flyoutpanels/AboutFlyoutPanel.js
@@ +250,5 @@
>    setupUpdateButton: function(aKeyPrefix) {
>      this.updateBtn.label = this.bundle.GetStringFromName(aKeyPrefix + ".label");
>      this.updateBtn.accessKey = this.bundle.GetStringFromName(aKeyPrefix + ".accesskey");
>      if (!document.commandDispatcher.focusedElement ||
> +        document.commandDispatcher.focusedElement != this.updateBtn)

actually, just realized this is a pointless double check. I thought I was fixing a focus bug here but maybe this was in place for some other reason? 

document.commandDispatcher.focusedElement != this.updateBtn

should suffice.

Comment 4

[Jim Mathies [:jimm]]

or really, just skip the if statement and set the focus to the button.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 833094 [details] [diff] [review]
fix

Review of attachment 833094 [details] [diff] [review]:
-----------------------------------------------------------------

Modulo the change I commented on, I think we're good to go.

::: browser/metro/base/content/flyoutpanels/AboutFlyoutPanel.js
@@ +250,5 @@
>    setupUpdateButton: function(aKeyPrefix) {
>      this.updateBtn.label = this.bundle.GetStringFromName(aKeyPrefix + ".label");
>      this.updateBtn.accessKey = this.bundle.GetStringFromName(aKeyPrefix + ".accesskey");
>      if (!document.commandDispatcher.focusedElement ||
> +        document.commandDispatcher.focusedElement != this.updateBtn)

I don't think we want a change here, this mirrors the code from:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#236

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 833094 [details] [diff] [review]
fix

This really looks like a bug to me - 

if (!document.commandDispatcher.focusedElement ||
    document.commandDispatcher.focusedElement == this.updateBtn)

If there's no focused element or the focused element equals the update button, set the focus to the update button? Gavin, is this right?

Comment 7

[Brian R. Bondy [:bbondy]]

What you're saying sounds right but I think you should double check with rstrong and I think it should be handled in another bug related to that problem. And in that bug fix the mirrored code in /browser' about dialog.

Comment 8

[Marco Mucci [:MarcoM]]

Hey Jim, can you provide a point value for this defect.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 833094 [details] [diff] [review]
fix

Certainly looks wrong, and bug 596813 comment 44 isn't any more enlightening. I'd ask Robert though.

Comment 10

[Jim Mathies [:jimm]]

I'm going to land this without the focus logic change since it looks pretty harmless. I'll split that out to another bug with a new patch.

Comment 11

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/a7d59b702986

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> Comment on attachment 833094 [details] [diff] [review]
> fix
> 
> Certainly looks wrong, and bug 596813 comment 44 isn't any more
> enlightening. I'd ask Robert though.
I agree that it looks wrong and I don't recall why it is that way. If it is really a noop and is working as is then it should be removed instead of the logic changed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7d59b702986

Comment 14

[Manuela Muntean [Away]]

Could you please provide some guidance for QA to test this? Thanks!

Comment 15

[Jim Mathies [:jimm]]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #14)
> Could you please provide some guidance for QA to test this? Thanks!

open settings, then open the about panel, then switch to the javascript console. You should not see the error in comment 1 anywhere.

Comment 16

[Manuela Muntean [Away]]

> open settings, then open the about panel, then switch to the javascript
> console. You should not see the error in comment 1 anywhere.

Following this steps, I confirm this is verified as fixed with latest Nightly from 2013-12-06 on Win 8 64-bit. The specified error doesn't appear in the javascript console.

On the other hand, while also trying this on Win 8.1 Pro 64-bit, I wasn't able to open the javascript console by using the Ctrl + Shift + J shortcut.

Does anyone have any suggestions/thoughts?

Comment 17

[Brian R. Bondy [:bbondy]]

I'm not sure that works for me (CTRL+SHIFT+J), you could try pinging one of the other people maybe on #windev to see i they have any extra ideas.

Comment 18

[Manuela Muntean [Away]]

Attachment: win 8.1_error console.png

I've retried the steps from comment 15 on Win 8.1 Pro 64-bit, with latest Nightly (build ID: 20131211030206), and I can see the error mentioned in comment 0. 

Please see the attached screenshot for details.