1140725 - Unreachable code after semicolon-less return statement in isDisabledForEvents.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Tooru Fujisawa [:arai]]

patch for bug 1005110 hits following warning.
> SyntaxError: unreachable code after semicolon-less return statement nsContextMenu.js:1529:6

https://hg.mozilla.org/mozilla-central/annotate/d9b06c673f80/browser/base/content/nsContextMenu.js#l1525
>   isDisabledForEvents: function(aNode) {
>     let ownerDoc = aNode.ownerDocument;
>     return
>       ownerDoc.defaultView &&
>       ownerDoc.defaultView
>               .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>               .getInterface(Components.interfaces.nsIDOMWindowUtils)
>               .isNodeDisabledForEvents(aNode);
>   },

Comment 1

[Tooru Fujisawa [:arai]]

Running try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7a78e63b62

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Fix unreachable code after semicolon-less return statement in nsContextMenu.js.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7a78e63b62
 (I guess the failure in linux asan M(oth) is not related to this patch)

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 8574319 [details] [diff] [review]
Fix unreachable code after semicolon-less return statement in nsContextMenu.js.

Thanks for the fix! r=jst, but someone who actually works on this code should stamp this too.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8574319 [details] [diff] [review]
Fix unreachable code after semicolon-less return statement in nsContextMenu.js.

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

Seems to suppress the context menu as originally intended for <input disabled>.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/48188cd36ed1

Comment 6

[Tooru Fujisawa [:arai]]

Backed out for mochitest failure on Windows and Mac.
I should've tested on all OS for this type of fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1add3c464199

test_contextmenu_input.html seems to expect menu for <input disabled>, so the test itself should be fixed.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Fix unreachable code after semicolon-less return statement in nsContextMenu.js. r=jst,dolske

test for <input disabled> in test_contextmenu_input.html is no more valid with the behavior change (originally in bug 816340).
could you review that part?

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf719bcf07a3

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 8575457 [details] [diff] [review]
Fix unreachable code after semicolon-less return statement in nsContextMenu.js. r=jst,dolske

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

Please go with one of the original reviewers.

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8575457 [details] [diff] [review]
Fix unreachable code after semicolon-less return statement in nsContextMenu.js. r=jst,dolske

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Please go with one of the original reviewers.

oops, sorry.

Comment 10

[Justin Dolske [:Dolske]]

Hum. Now that I think about this some more, and see the related bug 433168, I'm not sure the original addition of this code in 816340 was doing the right thing.

Suppressing the context menu entirely can be bad, because there's no feedback to the user when they attempt to activate it. It so happens most of the menuitems are disabled on disabled-elements, but at least you get feedback by seeing the menu. And, for some users, you'll have access to items provided by addons which may not be disabled (eg Inspect Element from our devtools, and in my case the Copy Short URL addon that globally adds a menu command for everything on the page.)

Smaug, am I missing some reason to make this code functional? (Since you wrote the patch in 816340.) If not, we should just entirely remove isDisabledForEvents() from nsContextMenu.

Tooru: sorry for adding more confusion here. :(

Comment 11

[Justin Dolske [:Dolske]]

Meant to add: as far as I can tell from bug 816340 comment 3, around the time that patch was written the context menu was actually _broken_ on disabled elements. And that's why it as being suppressed. But sometime between that claim and when this test was written, it apparently was fixed to work anyway?

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Remove isDisabledForEvents from nsContextMenu.js.

Oops, I landed a patch in bug 1005110 leaving this bug unresolved.
Prepared a patch that removes isDisabledForEvents :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dddda0166a

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 8587808 [details] [diff] [review]
Remove isDisabledForEvents from nsContextMenu.js.

Ugh, sorry this got dropped for so long. The minimal way to unblock this would be to add the semicolon to the return, comment out the already-dead code, and leave a bug open on figuring out what the original patch was supposed to do and how to properly fix things.

But seeing as I don't understand if the original fix was even correct (if it had worked), let's just remove the code as you've done here.

Leaving the needinfo on for Smaug in case he wants to investigate further.

Comment 15

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/b647d34b31b2

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #10)
> Smaug, am I missing some reason to make this code functional? (Since you
> wrote the patch in 816340.) If not, we should just entirely remove
> isDisabledForEvents() from nsContextMenu.
The reason was to not change the existing behavior, where event wasn't fired at all on disabled elements.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b647d34b31b2

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

...but apparently I managed to break the old behavior, but perhaps the current behavior is ok.