Closed Bug 1033463 Opened 6 years ago Closed 6 years ago

JS Error: "gEditItemOverlay is null" when using the keyboard to cancel "Bookmark all tabs" dialog

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.3

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(2 files)

Doesn't happen in beta, working on a regression window.
Happens on Windows as well as OS X. Not checked Linux.
OS: Mac OS X → All
Hardware: x86 → All
Flags: firefox-backlog+
I can't figure out a regression window because when I start with mozregression, I can't reproduce the bug, even on builds where I can reproduce on the same profile when not using mozregression. I don't know why that is.
Proper STR:

1. Open 2 or more tabs
2. Right click one
3. Click bookmark all tabs
4. hit [esc] on the keyboard to cancel the dialog

In particular, this shows the issue, whereas *clicking* the cancel button does *not*. which is what fooled me earlier today.

This goes back to pre-2013, so I don't know if it makes sense to try to find a regression window...
Keywords: helpwanted
onDialogCancel kills off gEditItemOverlay, and then the blur event caused by the keyboard event (or dialog closure, I'm not sure) fires, and then everyone is sad.
Attachment #8449840 - Flags: review?(mak77)
Summary: error: "gEditItemOverlay is null" when cancelling "Bookmark all tabs" dialog in nightly → JS Error: "gEditItemOverlay is null" when using the keyboard to cancel "Bookmark all tabs" dialog
Hi,

I would like to contribute on this bug. Can you please assign it to me?

Thanks in advance,

Regards,
Anup
(In reply to Anup Kumar from comment #5)
> Hi,
> 
> I would like to contribute on this bug. Can you please assign it to me?
> 
> Thanks in advance,
> 
> Regards,
> Anup

There's already a patch here...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8449840 [details] [diff] [review]
nullcheck gEditOverlay because it can be null if the dialog is cancelled,

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

::: browser/components/places/content/editBookmarkOverlay.xul
@@ +32,5 @@
>                   accesskey="&editBookmarkOverlay.name.accesskey;"
>                   control="editBMPanel_namePicker"
>                   observes="paneElementsBroadcaster"/>
>            <textbox id="editBMPanel_namePicker"
> +                   onblur="gEditItemOverlay &amp;&amp; gEditItemOverlay.onNamePickerChange();"

this would work, though editBookmarkOverlay.xul expects gEditItemOverlay to always exist since it's defined as a global in editBookmarkOverlay.js, and this overlay is reused in different parts of the ui.
The mistake here is that we set gEditItemOverlay = null in the bookmarkProperties dialog code, my assumption is that we do such thing as a shurtcut to ensure nothing more is modified from that point on. That shortcut is the wrong way to handle it though.

There are 2 alternatives to handle things more properly here:
1. don't use onblur, but rather addEventListener("blur") in editBookmarkOverlay.js::initPanel (and removeEventListener in uninitPanel)
2. check this._initialized in onNamePickerChange, onLocationFieldBlur, onTagsFieldBlur, onKeywordFieldBlur, onDescriptionFieldBlur.

Off-hand the first solution looks safer to me.
And in any case, we should stop nullifying gEditBookmarkOverlay, also in onDialogAccept.
Attachment #8449840 - Flags: review?(mak77) → feedback-
Marco, can you add this to the backlog?
Iteration: --- → 33.3
Points: --- → 2
Flags: needinfo?(mmucci)
Comment on attachment 8453827 [details] [diff] [review]
don't null out gEditBookmarkOverlay,

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

yes, this looks correct.
Attachment #8453827 - Flags: review?(mak77) → review+
Added to Iteration 33.3
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
QA Whiteboard: [qa?] → [qa+]
Try push with that test fixed: remote:   https://tbpl.mozilla.org/?tree=Try&rev=17879d4c9261
remote:   https://hg.mozilla.org/integration/fx-team/rev/0729de0a4a02
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0729de0a4a02
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: andrei.vaida
(this touched an automated test, but there's no specific coverage for this)
Flags: in-testsuite+ → in-testsuite-
I managed to reproduce this bug on Firefox 30.0 build2 (Build ID: 20140605174243), Aurora 32.0a2 (2014-07-14) and Firefox 31 Beta 9  (Build ID: 20140605174243) using Windows 7 64-bit. 

Verified fixed on Nightly 33.0a1 (2014-07-14).
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.