364994 - BiDi UI menuitems are always hidden in cocoa widget menus

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Asaf Romano (gone)]

The BiDi UI menuitems (Switch Text Direction / Switch Page Direction) are never visible in cocoa widget builds. Note these menuitems are initially disabled and are later enabled if bidi.browser.ui is set to true or if the system local is "bidi associated".

AFAICT, the dom state is correctly updated, but nsMenu*X doesn't pick up the changes.

Comment 1

[Josh Aas]

Attachment: do GetNativeData check, v1.0

This is a bug I found while fixing the bidi menu items, we shouldn't assume that we got a non-null value from GetNativeData.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 258337 [details] [diff] [review]
do GetNativeData check, v1.0

r=mano.

Comment 3

[Josh Aas]

"do GetNativeData check, v1.0" landed on trunk

Comment 4

[Josh Aas]

Attachment: cleanup 1, v1.0

My patch for this isn't quite ready, for now here is some more cleanup that will make the final patch more readable.

This cleanup as no functional changes whatsoever. This simply does the following, though it looks more complicated:

- a couple methods were marked as NS_IMETHODs when in fact they are not
- I made the following structural change in two functions to make them more clear. These functions are important for the actual patch to this bug. Instead of:

void function()
{
  if (x) {
    ...
  }
}

void function()
{
  if (!x)
    return;
  ...
}

Of course diff has to make this look much worse than it is...

Comment 5

[Asaf Romano (gone)]

Comment on attachment 258520 [details] [diff] [review]
cleanup 1, v1.0

whatever, r=mano.

Comment 6

[Stuart Parmenter]

can you post a diff -uw?

Comment 7

[Josh Aas]

Attachment: cleanup 1, v1.0, -w

Comment 8

[Josh Aas]

Attachment: fix v1.0

This fixes our handling of hidden menu items and thus fixes this bug.

Comment 9

[Josh Aas]

The gist of this patch is this - we need to keen nsIMenuItem objects around even for hidden menu items because otherwise we don't pick up attribute changes (the hidden and collapsed attributes in particular). This patch keeps the nsIMenuItem objects around and rebuilds the menu for the items when their hidden or collapsed attributes change.

Same goes for nsIMenu objects.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 259013 [details] [diff] [review]
fix v1.0

>Index: public/nsIMenu.h
>===================================================================

If we don't have a bug on DeCOMifying nsIMenu and friends, please file it and cc me.

>Index: public/nsIMenuItem.h
>===================================================================

>@@ -165,13 +165,19 @@ class nsIMenuItem : public nsISupports {
>     */
>     NS_IMETHOD SetModifiers(PRUint8 aModifiers) = 0;
>     NS_IMETHOD GetModifiers(PRUint8 * aModifiers) = 0;
> 
>    /**
>     * Sets an appropriate icon for the menu item.
>     */
>     NS_IMETHOD SetupIcon() = 0;
>+
>+    /**
>+     * Get GetMenuItemContent
>+     *
>+     */
>+    NS_IMETHOD GetMenuItemContent(nsIContent ** aMenuItemContent) = 0;
> };
> 

rev the uuid.

>Index: src/cocoa/nsMenuX.mm
>===================================================================

> NS_IMETHODIMP nsMenuX::AddItem(nsISupports* aItem)
> {
>   nsresult rv = NS_ERROR_FAILURE;
>-  if (aItem) {
>-    // Figure out what we're adding
>-    nsCOMPtr<nsIMenuItem> menuItem(do_QueryInterface(aItem));
>-    if (menuItem) {
>-      rv = AddMenuItem(menuItem);
>-    }
>-    else {
>-      nsCOMPtr<nsIMenu> menu(do_QueryInterface(aItem));
>-      if (menu)
>-        rv = AddMenu(menu);
>-    }
>+
>+  if (!aItem)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  // Figure out what we're adding
>+  nsCOMPtr<nsIMenuItem> menuItem(do_QueryInterface(aItem));
>+  if (menuItem) {

while you're at it, please change this to 

>+  nsCOMPtr<nsIMenuItem> menuItem(do_QueryInterface(aItem, &rv));
if (NS_SUCCEEDED(rv))
>+    rv = AddMenuItem(menuItem);

>   }
>+  else {
>+    nsCOMPtr<nsIMenu> menu(do_QueryInterface(aItem));
>+    if (menu)
>+      rv = AddMenu(menu);

ditto.

> void nsMenuX::LoadSeparator(nsIContent* inSeparatorContent) 
> {
>-  if (NodeIsHiddenOrCollapsed(inSeparatorContent))
>-    return;
>-
>-  AddSeparator();
>+  // Currently we don't create nsIMenuItem objects for separators so we can't
>+  // track changes in their hidden/collapsed attributes. If it is hidden now it
>+  // is hidden forever.

File the follow up about this as discussed over IRC.


r=mano otherwise, thanks josh!

Comment 11

[Josh Aas]

fixed on trunk

Comment 12

[Josh Aas]

I filed bug 375011 about the menu separator hiding stuff, Mano please file the
decom bug since you know what exactly you want to do.