589154 - Edit should be a label, not a menuitem

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Dão Gottwald [:dao]]

See bug 583386 comment 105.

Comment 1

[u88484]

Attachment: Patch v1

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 468032 [details] [diff] [review]
Patch v1

>-            <menuitem id="appmenu-edit-menuitem"
>-                      label="&editMenu.label;"
>-                      disabled="true"/>
>+            <label id="appmenu-edit-menuitem"
>+                   label="&editMenu.label;"
>+                   disabled="true"/>

The id should be changed to appmenu-edit-label or something like that.

The corresponding -moz-appearance: none; and background: transparent; in browser.css should be removed:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#248

Does disabled="true" pick up the gray text color for labels?

Comment 3

[Vlado Valastiak [:wladow]]

Since this is a label and not a proper menu item, please make it use its own label text rather than reusing editMenu.label. We need that to localize it properly.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 468032 [details] [diff] [review]
Patch v1

Yeah, comment 3 makes another good point. This is a slightly different context, so some locales might want to alter the string.

Comment 5

[u88484]

Attachment: Patch v2

(In reply to comment #2)
> Comment on attachment 468032 [details] [diff] [review]
> Patch v1
> 
> >-            <menuitem id="appmenu-edit-menuitem"
> >-                      label="&editMenu.label;"
> >-                      disabled="true"/>
> >+            <label id="appmenu-edit-menuitem"
> >+                   label="&editMenu.label;"
> >+                   disabled="true"/>
> 
> The id should be changed to appmenu-edit-label or something like that.
> 
Done

> The corresponding -moz-appearance: none; and background: transparent; in
> browser.css should be removed:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/browser.css#248
> 
Done
> Does disabled="true" pick up the gray text color for labels?
Yes

(In reply to comment #4)
> Comment on attachment 468032 [details] [diff] [review]
> Patch v1
> 
> Yeah, comment 3 makes another good point. This is a slightly different context,
> so some locales might want to alter the string.
Done

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 469193 [details] [diff] [review]
Patch v2

>                     command="Tools:PrivateBrowsing"/>
>           <menuseparator/>
>           <hbox class="split-menuitem">

Please remove this class as well.

>-            <menuitem id="appmenu-edit-menuitem"
>-                      label="&editMenu.label;"
>-                      disabled="true"/>
>+            <label id="appmenu-edit-label"
>+                   label="&appMenuEdit.label;"
>+                   disabled="true"/>

Does this keep the label aligned with the other menuitem labels? If not, it might be better to just change the id and the entity to make it clear that this isn't a menu item, conceptionally, but let it be a <menuitem> under the hood.

Comment 7

[u88484]

Attachment: Patch v3

It actually doesn't show at all once I change it from menuitem to label even if disabled="true" is removed or changed to false.  Uploading patch so you can see if I did something wrong or if we should just leave it as a menuitem

Comment 8

[Dão Gottwald [:dao]]

Yes, let's do this: change the id and the entity to make it clear that this
isn't a menu item, conceptionally, but let it be a <menuitem> under the hood.

Comment 9

[u88484]

Attachment: Patch v4

(In reply to comment #8)
> Yes, let's do this: change the id and the entity to make it clear that this
> isn't a menu item, conceptionally, but let it be a <menuitem> under the hood.

Done.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 470288 [details] [diff] [review]
Patch v4

Thanks!

Comment 11

[u88484]

Attachment: Patch v5

Unbitrotted

Comment 12

[u88484]

Attachment: Patch v6 for checkin

un-bitrotted patch for checkin

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fcae8913e466