-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolving Firefox MetricExplorer menu bug #114
Conversation
e759147
to
c4a6bce
Compare
Does this problem affect all Ext.menu.Menu component instances or just the one in metric explorer. If it affects all of them then surely the change should be made as an Ext js override so all instances have the fix applied. If it only affects the one in metric explorer then what is different about the metric explorer instance? |
**/ | ||
this.chartOptionsMenu.keyNav.left = function (e, m) { | ||
// Gecko === Firefox | ||
if (Ext.isGecko === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems wrong to me to have to check the rendering engine here. Why don't you just change the doRelay call to only relay the event if the keypress object has isNavKeyPress() === true?
- Typing a '%' character in a form field contained within the MetricExplorer chart options menu would not have the desired effect. Instead, the whole menu would close and the '%' character would not be recorded in the field. The reason for this is because the doRelay handler for the chartOptionsMenu.keyNav currently always relays events regardless of whether or not the event is an actual keyNav event or not. This updates that handler to only call the handler in the case the event is for a keyNav event, thus '%' is no longer interpreted as 'left'. This version of the fix was pointed out by a code review comment by @jpwhite4.
5e9e641
to
5668d69
Compare
- Typing a '%' character in a form field contained within the MetricExplorer chart options menu would not have the desired effect. Instead, the whole menu would close and the '%' character would not be recorded in the field. The reason for this is because the doRelay handler for the chartOptionsMenu.keyNav currently always relays events regardless of whether or not the event is an actual keyNav event or not. This updates that handler to only call the handler in the case the event is for a keyNav event, thus '%' is no longer interpreted as 'left'. This version of the fix was pointed out by a code review comment by @jpwhite4.
Description
MetricExplorer chart options menu would not have the desired effect. Instead,
the whole menu would close and the '%' character would not be recorded in the
field. The reason for this was because of an interaction between the FireFox
browser and the Ext.KeyNav / Ext.MenuNav components. In FireFox, when the '%'
character was entered into a form field contained within a menu, the
Ext.KeyNav.relay function would fire, interpret the keyCode value of '37' to
be the 'left' key, for which the Ext.MenuNav component has a handler. This
handler would then merrily went about closing the menu ( because that's what
you do when you press the left key? I'm guessing to support navigating a menu
/ toolbar via the arrow keys. ). Now, when Ext identifies the browser as
Gecko ( which is what Firefox is identified as ), it just returns true so as
to not interrupt any further events that need to be interpreted. If the
browser is not Firefox then it executes the same code as in the original
handler.
Motivation and Context
Attempting to enter a '%' character while using Firefox into the name / title text boxes in the Metric Explorer would result in the menu that contains the text boxes to be closed and the character text box values to not be updated.
Tests performed
Types of changes
Checklist: