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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[:Gijs (he/him)]

Doesn't happen in beta, working on a regression window.

Comment 1

[:Gijs (he/him)]

Happens on Windows as well as OS X. Not checked Linux.

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[:Gijs (he/him)]

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...

Comment 4

[:Gijs (he/him)]

Attachment: nullcheck gEditOverlay because it can be null if the dialog is cancelled,

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.

Comment 5

[Anup Kumar [:reznord]]

Hi,

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

Thanks in advance,

Regards,
Anup

Comment 6

[:Gijs (he/him)]

(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...

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[:Gijs (he/him)]

Attachment: don't null out gEditBookmarkOverlay,

Like this? (try push: https://tbpl.mozilla.org/?tree=Try&rev=80f3a4961631 )

Comment 9

[:Gijs (he/him)]

Marco, can you add this to the backlog?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8453827 [details] [diff] [review]
don't null out gEditBookmarkOverlay,

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

yes, this looks correct.

Comment 11

[Marco Mucci [:MarcoM]]

Added to Iteration 33.3

Comment 12

[:Gijs (he/him)]

Try push with that test fixed: remote:   https://tbpl.mozilla.org/?tree=Try&rev=17879d4c9261

Comment 13

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/0729de0a4a02

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0729de0a4a02

Comment 15

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 16

[:Gijs (he/him)]

(this touched an automated test, but there's no specific coverage for this)

Comment 17

[Andrei Vaida [:avaida]]

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).