Closed Bug 946166 Opened 7 years ago Closed 7 years ago

Clicking on disabled non-view widgets closes the panel

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: u428464, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa-])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [Australis:P?]
(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)
Flags: needinfo?(ge3k0s)
No feed button is fine. AFAIK it only happens with cut/copy/paste
Flags: needinfo?(ge3k0s)
Confirmed 29.0a1 (2013-12-11), Win 7 x64.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Clicking on disabled cut/copy/paste button close the panel → Clicking on disabled cut/copy/paste button and zoom widget borders close the panel
Whiteboard: [Australis:P?] → [Australis:P5]
Duplicate of this bug: 963286
(Quoting Philipp Sackl [:phlsa] from bug 963286 comment 2)
> You're right, if no action is triggered, the panel shouldn't close.
Summary: Clicking on disabled cut/copy/paste button and zoom widget borders close the panel → Clicking on disabled cut/copy/paste button and zoom widget borders closes the panel
Duplicate of this bug: 978460
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.
Summary: Clicking on disabled cut/copy/paste button and zoom widget borders closes the panel → Clicking on disabled non-view widgets closes the panel
Whiteboard: [Australis:P5] → [Australis:P4]
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.
Because the 2nd point mentioned in my previous comment, this bug should block bug 976420.
Duplicate of this bug: 1005691
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.
Flags: firefox-backlog+
Simple patch + test (verified the test fails without the code change)
Attachment #8423155 - Flags: review?(mdeboer)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Marco, can you add this to this sprint's backlog?
Flags: needinfo?(mmucci)
Whiteboard: [Australis:P4] → [Australis:P4] p=2 s=it-32c-31a-30b.2
Thanks Gijs, iteration updated.
Flags: needinfo?(mmucci)
Whiteboard: [Australis:P4] p=2 s=it-32c-31a-30b.2 → [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa?]
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.
Attachment #8423155 - Flags: review?(mdeboer)
Attachment #8423312 - Flags: review?(mdeboer)
Attachment #8423155 - Attachment is obsolete: true
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.
Attachment #8423312 - Flags: review?(mdeboer)
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...
Attachment #8423312 - Flags: review?(mdeboer)
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!
Attachment #8423312 - Flags: review?(mdeboer) → review+
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.
Attachment #8423838 - Flags: review?(mdeboer)
Attachment #8423312 - Attachment is obsolete: true
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 :)
Attachment #8423838 - Flags: review?(mdeboer) → review+
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
https://hg.mozilla.org/integration/fx-team/rev/0fa6473248da
Whiteboard: [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa?] → [Australis:P4][fixed-in-fx-team] p=2 s=it-32c-31a-30b.2 [qa?]
https://hg.mozilla.org/mozilla-central/rev/0fa6473248da
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] p=2 s=it-32c-31a-30b.2 [qa?] → [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa?]
Target Milestone: --- → Firefox 32
Whiteboard: [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa?] → [Australis:P4] p=2 s=it-32c-31a-30b.2 [qa-]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.