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

RESOLVED FIXED in Firefox 28

Status

P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 28
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 833094 [details] [diff] [review]
fix
Assignee: nobody → jmathies
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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: 936489
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
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Could you please provide some guidance for QA to test this? Thanks!
Flags: needinfo?(netzen)
(Assignee)

Comment 15

5 years ago
(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)
(Assignee)

Updated

5 years ago
Flags: needinfo?(jmathies)

Updated

5 years ago
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)
Created attachment 8346477 [details]
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.
You need to log in before you can comment on or make changes to this bug.