946166 - Clicking on disabled non-view widgets closes the panel

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

When the cut/copy/paste wide widget is in the menu panel and you click on it in disabled state it closes the panel. I think it should work like blank area and not close.

Comment 1

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #1)
> When the cut/copy/paste wide widget is in the menu panel and you click on it
> in disabled state it closes the panel. I think it should work like blank
> area and not close.

Does the same not happen with all the other widgets if they are disabled? (feed button is probably easy to test)

Comment 2

[u428464]

No feed button is fine. AFAIK it only happens with cut/copy/paste

Comment 3

[Paul Silaghi, QA [:pauly]]

Confirmed 29.0a1 (2013-12-11), Win 7 x64.

Comment 5

[Matthew N. [:MattN]]

(Quoting Philipp Sackl [:phlsa] from bug 963286 comment 2)
> You're right, if no action is triggered, the panel shouldn't close.

Comment 7

[:Gijs (he/him)]

The feed and character encoding widgets both have a subview, which is why clicking them doesn't close the panel even if they're disabled. Otherwise, this is broken for all other items.

Comment 8

[Carlos [nohamelin]]

Some extra observations:

* Clicking on a disabled menuitem in a subview hides the panel too (Try with "Restore Previous Session" in the History widget).
* Clicking on a disabled menuitem in a menu from a xul toolbarbutton (type="menu") hides the parent panel, but the xul menu is left visible.

Comment 9

[Carlos [nohamelin]]

Because the 2nd point mentioned in my previous comment, this bug should block bug 976420.

Comment 11

[:Gijs (he/him)]

We should just fix this. All that needs to happen is that this function: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#1325 needs to be taught about the disabled attribute on nodes, and return 'true' in those cases, instead of false.

Comment 12

[:Gijs (he/him)]

Attachment: make clicking disabled items not close the panel,

Simple patch + test (verified the test fails without the code change)

Comment 13

[:Gijs (he/him)]

Marco, can you add this to this sprint's backlog?

Comment 14

[Marco Mucci [:MarcoM]]

Thanks Gijs, iteration updated.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8423155 [details] [diff] [review]
make clicking disabled items not close the panel,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1389,5 @@
>          menuitemCloseMenu = (closemenuVal == "single" || closemenuVal == "none") ?
>                              closemenuVal : "auto";
>        }
> +      if (inButton || isMenuItem || inInput) {
> +        isDisabled = target.getAttribute("disabled") == "true";

Isn't this a bit too defensive? I mean, what could go wrong if we'd simply say:

```js
if (target.getAttribute("disabled") == "true")
  return true;
```
?

::: browser/components/customizableui/test/browser_940307_panel_click_closure_handling.js
@@ +92,5 @@
>  });
>  
> +add_task(function*() {
> +  button = document.createElement("toolbarbutton");
> +  button.id = "browser_940307_button_disabled";

nit: you might want to make this id mention '946166' instead.

Comment 16

[:Gijs (he/him)]

Attachment: make clicking disabled items not close the panel,

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8423312 [details] [diff] [review]
make clicking disabled items not close the panel,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1407,5 @@
> +
> +    // If the user clicked on a disabled item, we definitely need to leave the menu open.
> +    if (isDisabled) {
> +      return true;
> +    }

Since we bail out here, it's not necessary to do the bookkeeping in an `isDisabled` variable, right? I think you can just do

```js
if (target.getAttribute("disabled") == "true")
  return true;
```

...to be the only change this patch makes.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8423312 [details] [diff] [review]
make clicking disabled items not close the panel,

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

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Comment on attachment 8423312 [details] [diff] [review]
> make clicking disabled items not close the panel,
> 
> Review of attachment 8423312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +1407,5 @@
> > +
> > +    // If the user clicked on a disabled item, we definitely need to leave the menu open.
> > +    if (isDisabled) {
> > +      return true;
> > +    }
> 
> Since we bail out here, it's not necessary to do the bookkeeping in an
> `isDisabled` variable, right? I think you can just do
> 
> ```js
> if (target.getAttribute("disabled") == "true")
>   return true;
> ```
> 
> ...to be the only change this patch makes.

No, because target changes all the time. If I'm on a disabled item inside another item, and we don't break the loop until we hit the other item, just comparing at the end of the loop will return false. Hence the logical 'or' inside the loop...

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8423312 [details] [diff] [review]
make clicking disabled items not close the panel,

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

Ah, right! Thanks for explaining that.

Good work, ship it!

Comment 20

[:Gijs (he/him)]

Attachment: make clicking disabled items not close the panel,

Actually, thinking about this more, I was somewhat wrong. As soon as the value is true, we can bail from inside the loop, like this.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8423838 [details] [diff] [review]
make clicking disabled items not close the panel,

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

Alright, yeah, that's what I meant :)

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8423838 [details] [diff] [review]
make clicking disabled items not close the panel,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1406,5 @@
>        } else {
>          target = target.parentNode;
>        }
>      }
> +

nit: newline not strictly necessary

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/0fa6473248da

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fa6473248da