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

RESOLVED FIXED in Firefox 4.0b8

Status

()

RESOLVED FIXED
8 years ago
8 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)

(Reporter)

Description

8 years ago
The binding from bug 589139 isn't applied on Linux, need to fix that.
(Reporter)

Updated

8 years ago
Blocks: 585370
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Updated

8 years ago
Component: Menus → Widget: Gtk
Product: Firefox → Core
QA Contact: menus → gtk
(Assignee)

Comment 2

8 years ago
I am pretty sure this is actually a native themeing gtk widget issue.
(Assignee)

Comment 3

8 years ago
(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.
(Reporter)

Updated

8 years ago
Assignee: nobody → dao
Component: Widget: Gtk → General
Product: Core → Firefox
QA Contact: gtk → general
(Assignee)

Comment 4

8 years ago
Created attachment 484675 [details] [diff] [review]
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.
Assignee: dao → bill
Status: NEW → ASSIGNED
Attachment #484675 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
Attachment #484675 - Attachment description: woraround → workaround
(Assignee)

Comment 5

8 years ago
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)
(Reporter)

Comment 6

8 years ago
I think I know fairly well what needs to be done here, which is why I assigned this to me...
(Assignee)

Comment 7

8 years ago
Created attachment 484867 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Assignee: bill → dao
(Reporter)

Comment 8

8 years ago
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
(Assignee)

Comment 9

8 years ago
Created attachment 486340 [details] [diff] [review]
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.
Assignee: dao → bill
Attachment #484867 - Attachment is obsolete: true
Attachment #486340 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
Attachment #486340 - Attachment description: patch v2 → patch v3
(Assignee)

Comment 10

8 years ago
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/
(Reporter)

Comment 11

8 years ago
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-
(Assignee)

Comment 12

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
(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.
(Assignee)

Comment 14

8 years ago
Created attachment 486663 [details] [diff] [review]
patch v4-address review comments
Attachment #486340 - Attachment is obsolete: true
Attachment #486663 - Flags: review?(dao)
(Reporter)

Updated

8 years ago
Attachment #486663 - Flags: review?(dao) → review+
(Assignee)

Updated

8 years ago
Attachment #486663 - Flags: approval2.0?
Attachment #486663 - Flags: approval2.0? → approval2.0+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8ce8499afe38
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.