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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

The binding from bug 589139 isn't applied on Linux, need to fix that.

Comment 1

[Bill Gianopoulos [:WG9s]]

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.

Comment 2

[Bill Gianopoulos [:WG9s]]

I am pretty sure this is actually a native themeing gtk widget issue.

Comment 3

[Bill Gianopoulos [:WG9s]]

(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.

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: workaround

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.

Comment 5

[Bill Gianopoulos [:WG9s]]

Comment on attachment 484675 [details] [diff] [review]
workaround

Withdrawing this patch.  Seems this is in some ways easier, in other ways harder.

Comment 6

[Dão Gottwald [:dao]]

I think I know fairly well what needs to be done here, which is why I assigned this to me...

Comment 7

[Bill Gianopoulos [:WG9s]]

Attachment: workaround-v2

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.

Comment 8

[Dão Gottwald [: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

Comment 9

[Bill Gianopoulos [:WG9s]]

Attachment: patch v3

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.

Comment 10

[Bill Gianopoulos [:WG9s]]

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 11

[Dão Gottwald [:dao]]

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?

Comment 12

[Bill Gianopoulos [:WG9s]]

(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.

Comment 13

[Bill Gianopoulos [:WG9s]]

(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.

Comment 14

[Bill Gianopoulos [:WG9s]]

Attachment: patch v4-address review comments

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8ce8499afe38