Last Comment Bug 772808 - menuitems don't inherit the "hidden" attribute from commands
: menuitems don't inherit the "hidden" attribute from commands
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Neil Deakin
:
Mentors:
Depends on: 806781
Blocks: 805653 808377
  Show dependency treegraph
 
Reported: 2012-07-11 05:38 PDT by Paul Rouget [:paul]
Modified: 2012-11-05 17:53 PST (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.52 KB, patch)
2012-08-31 09:17 PDT, Neil Deakin
neil: review+
smichaud: review+
mixedpuppy: review+
Details | Diff | Review

Description Paul Rouget [:paul] 2012-07-11 05:38:26 PDT
<command id="c1" hidden="true">
<broadcaster id="b1" hidden="true">

<toolbarbutton command="c1">
<menuitem1 observes="b1">
<menuitem2 command="c1">

toolbarbutton is hidden.
menuitem1 is hidden.
menuitem2 is not hidden.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-11 12:51:13 PDT
Is this Mac-specific?
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-30 11:53:42 PDT
I'm seeing it on Windows as well.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-30 17:22:12 PDT
Neil: any ideas here?
Comment 4 Neil Deakin 2012-08-31 09:17:58 PDT
Created attachment 657318 [details] [diff] [review]
Patch

Well, one idea is to implement this for menuitems.
Comment 5 neil@parkwaycc.co.uk 2012-09-04 04:55:34 PDT
Comment on attachment 657318 [details] [diff] [review]
Patch

Seems reasonable to me, although I don't have a Mac to test that change.
Comment 6 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-09-28 12:34:19 PDT
Comment on attachment 657318 [details] [diff] [review]
Patch

Shane, can you test this out on Mac? It would be good to have so we can clean up http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#96
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-09-28 12:42:09 PDT
I won't be able to get to this until (maybe) sometime next week.

So I'd be happy if Shane can test and review.
Comment 8 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-28 15:28:23 PDT
I'll take a look at it soon, busy building...
Comment 9 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-28 16:09:38 PDT
Comment on attachment 657318 [details] [diff] [review]
Patch

patch looks fine, tests pass, and a quick patch on socialapi shows that this allows us to resolve the issue jaws mentions in comment 6.
Comment 10 Steven Michaud [:smichaud] (Retired) 2012-10-08 14:03:33 PDT
Comment on attachment 657318 [details] [diff] [review]
Patch

This looks fine to me.

I saw no problems testing it on OS X 10.7.5.

I right-clicked in a bunch of different contexts, and always got an appropriate result.  I also ran the included test manually a bunch of times, and saw no failures.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-10-19 19:03:59 PDT
https://hg.mozilla.org/mozilla-central/rev/9b7c3072b2a3
Comment 12 Mark Hammond [:markh] 2012-10-22 22:20:54 PDT
I'm having some trouble with this which I think is related to the fact that having a hidden attribute with *any* value acts as though hidden="true".  So the only way to have the command work as though it is not hidden is to remove the attribute from the command.  But if that is done, the patch doesn't update the hidden command on the menu item.

ie, there seems no way to remove the hidden attribute from the menuitem using just commands.
Comment 13 Mark Hammond [:markh] 2012-10-23 00:19:13 PDT
hrm - please ignore the previous comment - XUL elements do want explicitly hidden="true".  I'm not sure what problems I'm seeing but they don't seem to be related to this afaict.

Note You need to log in before you can comment on or make changes to this bug.