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


(Firefox :: Bookmarks & History, defect)

Not set



Firefox 33


(Reporter: Gijs, Assigned: Gijs)



(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

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

Thanks in advance,

(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
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="&;"
>                   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:
Whiteboard: [fixed-in-fx-team]
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).
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.