Closed Bug 1033463 Opened 6 years ago Closed 6 years ago
JS Error: "g
Edit Item Overlay is null" when using the keyboard to cancel "Bookmark all tabs" dialog
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
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...
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 && 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-
Like this? (try push: https://tbpl.mozilla.org/?tree=Try&rev=80f3a4961631 )
Attachment #8453827 - Flags: review?(mak77)
Marco, can you add this to the backlog?
Iteration: --- → 33.3
Points: --- → 2
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?]
Try push with that test fixed: remote: https://tbpl.mozilla.org/?tree=Try&rev=17879d4c9261
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification of this bug.
(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.