Closed Bug 589154 Opened 12 years ago Closed 12 years ago

Edit should be a label, not a menuitem

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b7

People

(Reporter: dao, Assigned: u88484)

References

Details

(Whiteboard: [strings])

Attachments

(3 files, 3 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → supernova00
Status: NEW → ASSIGNED
Attachment #468032 - Flags: review?(dao)
OS: Windows XP → All
Hardware: x86 → All
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?
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 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.
Attachment #468032 - Flags: review?(dao) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
(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
Attachment #468032 - Attachment is obsolete: true
Attachment #469193 - Flags: review?(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.
Attached patch Patch v3 (obsolete) — Splinter Review
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
Attachment #469625 - Flags: review?(dao)
Attachment #469193 - Attachment is obsolete: true
Attachment #469193 - Flags: review?(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.
Attachment #469625 - Flags: review?(dao)
Attached patch Patch v4Splinter Review
(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.
Attachment #469625 - Attachment is obsolete: true
Attachment #470288 - Flags: review?(dao)
Comment on attachment 470288 [details] [diff] [review]
Patch v4

Thanks!
Attachment #470288 - Flags: review?(dao) → review+
Attachment #470288 - Flags: approval2.0?
Whiteboard: [strings]
Attached patch Patch v5Splinter Review
Unbitrotted
Attachment #471982 - Flags: approval2.0?
Attachment #470288 - Flags: approval2.0?
Attachment #471982 - Flags: approval2.0? → approval2.0+
un-bitrotted patch for checkin
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/fcae8913e466
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
You need to log in before you can comment on or make changes to this bug.