Resolving Firefox MetricExplorer menu bug #114
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanrath
force-pushed
the
metric_explorer_left_key
branch
from
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? |
jpwhite4
reviewed
Apr 26, 2017
| **/ | ||
| this.chartOptionsMenu.keyNav.left = function (e, m) { | ||
| // Gecko === Firefox | ||
| if (Ext.isGecko === true) { |
There was a problem hiding this comment.
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.
ryanrath
force-pushed
the
metric_explorer_left_key
branch
from
5e9e641
to
5668d69
Compare
jpwhite4
approved these changes
Apr 27, 2017
chakrabortyr
pushed a commit
to chakrabortyr/xdmod
that referenced
this pull request
Oct 17, 2017
- 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: