Make accelerator-text-in-tooltip work for top-level app menu items on Linux

RESOLVED FIXED in Firefox 4.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dao, Assigned: wgianopoulos)

Tracking

Trunk
Firefox 4.0b8
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The binding from bug 589139 isn't applied on Linux, need to fix that.
Blocks: 585370
I am not sure this is the entire issue here, as the menu seems to display fine, without showing the accelerator keys just as desired, if you use a third-party theme.
Component: Menus → Widget: Gtk
Product: Firefox → Core
QA Contact: menus → gtk
I am pretty sure this is actually a native themeing gtk widget issue.
(In reply to comment #2)
> I am pretty sure this is actually a native themeing gtk widget issue.

I should have said.  not only does this not work, but my attempts to do this by adding a new class on the menuitems on which I wanted the accelerators to be hidden fails to match in cases where it would seem it should.  It all works correctly if using anything other than the default linux theme which means it only fails when trying to use native gtk widgets.
Assignee: nobody → dao
Component: Widget: Gtk → General
Product: Core → Firefox
QA Contact: gtk → general
Posted patch workaround (obsolete) — Splinter Review
I see Dao already set this back to Firefox General before I had a chance to.  Although this seems to be a native widget issue, because it all works correctly under Linux if you install any third party theme and fails only under gnomestripe which uses native themeing for menus, the workaround I have come up with is a theme change.

I made up a new class called hide_accel and applied this to the menu items for which we don't want the accelerator text appear, then set added a css rule so to not display the accelerator text if this class is applied to the menuitem.

I am really not at all sure this is a proper fix or more of a workaround.

This patch applies on top of the pending patch on bug 585370.
Assignee: dao → bill
Status: NEW → ASSIGNED
Attachment #484675 - Flags: review?(dao)
Attachment #484675 - Attachment description: woraround → workaround
Comment on attachment 484675 [details] [diff] [review]
workaround

Withdrawing this patch.  Seems this is in some ways easier, in other ways harder.
Attachment #484675 - Attachment is obsolete: true
Attachment #484675 - Flags: review?(dao)
I think I know fairly well what needs to be done here, which is why I assigned this to me...
Posted patch workaround-v2 (obsolete) — Splinter Review
Great, because obviously my workaround only does half the job, hides the accelerator from the menuitem, but does not do the tooltip.  That obvioulsy requires getting the binding to work.

Attaching the better version anyway for reference.
Assignee: bill → dao
The reason why the binding isn't applied is this code:

235 /* Stock icons for the menu bar items */
236 menuitem:not([type]) {
237   -moz-binding: url("chrome://global/content/bindings/menu.xml#menuitem-iconic");
238 }

http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/browser.css#235
Posted patch patch v3 (obsolete) — Splinter Review
This ended up being simpler than I expected to fix.

Since this is related to, and was initially split off from, bug 585370 which has still not landed, and the patch conflicts with the pending one in that bug, I would like to add this code to the patch there.

The reason for this is that I don;t have check-in privileges and by combining this all back into the other bug, it will be more likely to land correctly.
Assignee: dao → bill
Attachment #484867 - Attachment is obsolete: true
Attachment #486340 - Flags: review?(dao)
Attachment #486340 - Attachment description: patch v2 → patch v3
I should mention for people who want to test this, I have test builds including both patches available at http://www.wg9s.com/mozilla/firefox/
Comment on attachment 486340 [details] [diff] [review]
patch v3

Just changing the selector from menuitem:not([type]) to menuitem:not([type]):not(.menuitem-tooltip):not(.menuitem-iconic-tooltip) should be sufficient, no?
Attachment #486340 - Flags: review?(dao) → review-
(In reply to comment #11)
> Comment on attachment 486340 [details] [diff] [review]
> patch v3
> 
> Just changing the selector from menuitem:not([type]) to
> menuitem:not([type]):not(.menuitem-tooltip):not(.menuitem-iconic-tooltip)
> should be sufficient, no?

That's what I get for fixing this in userChrome.css first and then just copying the code.  I believe that should work.  Is it ok for me to include this in the other bug and just way this bug will be fixed by the patch for that one because of the merge conflict issue?  Since I have no idea in what order these are liiely to land.
(In reply to comment #12)
> the code.  I believe that should work.  Is it ok for me to include this in the
> other bug and just way this bug will be fixed by the patch for that one because
> of the merge conflict issue?  Since I have no idea in what order these are
> liiely to land.

Nevermind, I think doing it your way avoids the merge conflict I think becauae there should be enough matching context after the change.
Attachment #486340 - Attachment is obsolete: true
Attachment #486663 - Flags: review?(dao)
Attachment #486663 - Flags: review?(dao) → review+
Attachment #486663 - Flags: approval2.0?
Attachment #486663 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8ce8499afe38
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
You need to log in before you can comment on or make changes to this bug.