Closed Bug 939192 Opened 11 years ago Closed 11 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [block28] feature=defect c=tbd u=tbd p=1)

Attachments

(2 files)

[JavaScript Error: "ReferenceError: AboutFlyout is not defined" {file: "chrome://browser/content/flyoutpanels/AboutFlyoutPanel.js" line: 86}]
Attached patch fixSplinter Review
Assignee: nobody → jmathies
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.
Attachment #833094 - Flags: review?(netzen)
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.
or really, just skip the if statement and set the focus to the button.
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
Attachment #833094 - Flags: review?(netzen) → review+
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?
Attachment #833094 - Flags: feedback?(gavin.sharp)
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.
Hey Jim, can you provide a point value for this defect.
Blocks: metrov1it19
Status: NEW → ASSIGNED
Flags: needinfo?(jmathies)
Priority: -- → P2
QA Contact: jbecerra
Summary: error in sync settings: "ReferenceError: AboutFlyout is not defined" → Defect - error in sync settings: "ReferenceError: AboutFlyout is not defined"
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0
Flags: needinfo?(jmathies)
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=1
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.
Attachment #833094 - Flags: feedback?(gavin.sharp) → feedback?(robert.bugzilla)
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.
Attachment #833094 - Flags: feedback?(robert.bugzilla)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
https://hg.mozilla.org/mozilla-central/rev/a7d59b702986
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Could you please provide some guidance for QA to test this? Thanks!
Flags: needinfo?(netzen)
(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.
Flags: needinfo?(netzen)
> 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?
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Flags: needinfo?(netzen)
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.
Flags: needinfo?(netzen)
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: