Fix focus issues by wldcordeiro · Pull Request #3505 · firefox-devtools/debugger

wldcordeiro · 2017-07-31T06:41:30Z

Associated Issue: #3495

Summary of Changes

Fixes the focus issue across all search inputs. They now take focus when enabled.
Bump yarn.lock to match updated devtools-reps version
CSS got reformatted by prettier?

Test Plan

Example test plan:

Command-P opens the panel with focus
Command-F opens search panel with focus
Comman-Shift-O opens symbol search with focus

codecov · 2017-07-31T06:55:11Z

Codecov Report

Merging #3505 into master will increase coverage by 0.08%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #3505      +/-   ##
==========================================
+ Coverage   54.35%   54.44%   +0.08%     
==========================================
  Files         119      119              
  Lines        4660     4662       +2     
  Branches      962      962              
==========================================
+ Hits         2533     2538       +5     
+ Misses       2127     2124       -3

Impacted Files	Coverage Δ
src/components/shared/SearchInput.js	`92.59% <100%> (+0.92%)`	⬆️
src/components/shared/Modal.js	`88.23% <100%> (+19.81%)`	⬆️
src/components/SymbolModal.js	`43.07% <33.33%> (-1.11%)`	⬇️
src/actions/ast.js	`78.57% <0%> (+1.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a90c35...2188a50. Read the comment docs.

codehag

Great work, I like this pattern, using refs instead of using componentDidMount + reactDom.findDOMNode for associating a node to the component. I think we should use it elsewhere too!

A couple of comments, and can you also add the test from the issue?

codehag · 2017-07-31T07:18:59Z

  static defaultProps: Object;

+  componentDidMount() {
+    this.input.focus();


codehag · 2017-07-31T07:31:30Z


+  componentDidMount() {
+    this.input.focus();
+    this.input.select();


In the old code, we used setSelectionRange -- was this to move the cursor to the end of the input? I also noticed that in both cases we only fire select when the component is opened, and in that case it is empty. perhaps we don't need this?

We were using it but we were making a selection of the end to the end? So I wasn't sure what exactly that was achieving. I don't think it nor .select() make a ton of sense so I don't care for keeping them.

wldcordeiro · 2017-07-31T08:02:40Z

can you also add the test from the issue?

Sure! I forgot that the issue had a test associated. Thanks @codehag

wldcordeiro · 2017-07-31T08:54:33Z

  pressKey(dbg, "sourceSearch");
  yield waitForElement(dbg, "input");
-  findElementWithSelector(dbg, "input").focus();
+  ok(


Correct me if I'm wrong but this is the way to assert in mochitest?

jasonLaster

couple of comments, but overall this is great!

jasonLaster · 2017-07-31T14:00:18Z

        value: query,
        spellCheck: false,
-        ref: "input"
+        ref: c => (this.input = c)


is this the preferred style?

perhaps? ref: c => (this.$input = c)

it's funny, i prefer refs because their namespaced, but i'll support anything.

Also this style is fine w/ me too
https://github.com/devtools-html/debugger.html/blob/master/src/components/shared/Autocomplete.js#L51-L53

jasonLaster · 2017-07-31T14:00:18Z

+    if (
+      this.refs.searchInput &&
+      this.refs.searchInput.input &&
+      this.props.enabled


nit: but lets put this.props.enabled first

jasonLaster · 2017-07-31T14:00:18Z

+
+    if (selectedSource && enabled) {
+      modalEnabled = true;
+    }


we might be playing too much of a dance w/ enabled, perhaps a new method is in order:

we dont need enabled prop

we can consolidate logic

isEnabled() { const {activeSearch, selectedSource} = this.props return activeSearch === "symbol" && selectedSource }

So enabled is also used for whether to update the results or not. I figured that I could instead modify the value we get in the connect call.

export default connect( state => { const source = getSelectedSource(state); return { enabled: getActiveSearchState(state) === "symbol" && source, symbolType: getSymbolSearchType(state), symbols: _getFormattedSymbols(state, source) }; }, dispatch => bindActionCreators(actions, dispatch) )(SymbolModal);

jasonLaster · 2017-07-31T14:00:18Z

  pressKey(dbg, "sourceSearch");
  yield waitForElement(dbg, "input");
-  findElementWithSelector(dbg, "input").focus();
+  ok(


yep, that will work!

i guess i was assuming it because the next action is typing... and it will work or fail based on that, but i don't care. this is nice to have.

jasonLaster

couple of comments, but overall this is great!

jasonLaster · 2017-07-31T14:00:24Z

        value: query,
        spellCheck: false,
-        ref: "input"
+        ref: c => (this.input = c)


is this the preferred style?

perhaps? ref: c => (this.$input = c)

it's funny, i prefer refs because their namespaced, but i'll support anything.

Also this style is fine w/ me too
https://github.com/devtools-html/debugger.html/blob/master/src/components/shared/Autocomplete.js#L51-L53

Are you suggesting that we use the $ to prefix it as a ref?

i prefer $ if we do the property style. I still like ReactDOM.findNode or this.ref as well.

Oh I could put the ref into the refs object as well by changing the line to this.refs.input = c

Correction. Seems like you can't add the property to the refs object via a function ref.

jasonLaster · 2017-07-31T14:00:24Z

+    if (
+      this.refs.searchInput &&
+      this.refs.searchInput.input &&
+      this.props.enabled


nit: but lets put this.props.enabled first

jasonLaster · 2017-07-31T14:00:24Z

+
+    if (selectedSource && enabled) {
+      modalEnabled = true;
+    }


we might be playing too much of a dance w/ enabled, perhaps a new method is in order:

we dont need enabled prop

we can consolidate logic

isEnabled() { const {activeSearch, selectedSource} = this.props return activeSearch === "symbol" && selectedSource }

jasonLaster · 2017-07-31T14:00:24Z

  pressKey(dbg, "sourceSearch");
  yield waitForElement(dbg, "input");
-  findElementWithSelector(dbg, "input").focus();
+  ok(


yep, that will work!

i guess i was assuming it because the next action is typing... and it will work or fail based on that, but i don't care. this is nice to have.

jasonLaster · 2017-07-31T18:17:44Z


+  componentDidUpdate() {
+    const shortcuts = this.context.shortcuts;
+    if (this.props.enabled) {


hmm, this worries me because we might be doing too many on calls.

Should we add a check in here like !props.enabled && nextProps.enabled?

I tried this and it wouldn't work after the first time you toggled on.

codehag · 2017-08-03T08:43:00Z

Rebased

codehag · 2017-08-03T11:32:00Z

Not sure what is happening with the tests; they pass on master and locally. looks like the problem is again with pretty print:


The following tests failed:

3522 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/head.js:382 - Error: Unable to find source: math.min.js:formatted

Stack trace:

    findSource@chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/head.js:382:11

    @chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-pretty-print.js:13:17

    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9

    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7

    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9

    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7

    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

3523 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-searching.js | focused on input - 

Stack trace:

    chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-searching.js:null:10

    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9

    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7

    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

3524 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-searching.js | focused on input - 

Stack trace:

    chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-searching.js:null:23

    Tester_execTest@chrome://mochikit/content/browser-test.js:735:9

    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7

    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Buffered messages finished

SUITE-END | took 150s


./bin/run-mochitests-docker returned exit code 1

codehag

I noticed a bit of an issue with a parent component controlling a child components behavior -- i think we should change that but overall it looks good

codehag · 2017-08-03T12:30:20Z

    const shortcuts = this.context.shortcuts;
    if (this.refs.searchInput) {
-      this.refs.searchInput.refs.input.focus();
+      this.refs.searchInput.$input.focus();


hmm this looks like it might give us trouble later on -- we are controlling a child component from a parent component; this makes it difficult to separate them -- we probably want searchInput to do this, rather than doing it here.

codehag · 2017-08-03T12:31:21Z

  static defaultProps: Object;

+  componentDidMount() {
+    this.$input.focus();


this should be enough... was it not working with this ?

Because the modal is mounted the symbol modal needs to have that if block to focus the input. I'll research further to see if there's a solution that's cleaner but this gets the focus events working on all the search dialogs and ensures we have no regressions on the shortcuts.

wldcordeiro · 2017-08-03T23:30:06Z

    "wasmparser": "^0.4.10"
  },
-  "files": ["src", "assets"],
+  "files": [


I think yarn reformats this file when you use yarn add 🤷‍♂️

wldcordeiro · 2017-08-03T23:34:05Z

 }

-.theme-dark .symbol-modal {
-  box-shadow: 2px 4px 6px var(--popup-shadow-color);


Redundant rule? It's on the modal itself just above.

wldcordeiro · 2017-08-04T01:19:45Z

  componentDidMount() {
    const shortcuts = this.context.shortcuts;
    shortcuts.on(L10N.getStr("symbolSearch.search.key2"), this.openSymbolModal);
+    shortcuts.on("Escape", this.closeModal);


It was problematic adding shortcuts to the modal itself and would leave the shortcut disabled on its dismount. Letting the consumer's handle it seems like a simpler API.

wldcordeiro · 2017-08-04T01:21:51Z

+  return Transition({
+    in: inProp,
+    timeout: 175,
+    appear: true,


Normally a component is not transitioned if it shown when the component mounts. If you want to transition on the first mount set appear to true, and the component will transition in as soon as the mounts.

This prop is awesome but sadly there doesn't seem to be a leave that is the equivalent. I've opened an issue on react-transition-group requesting one to see what they recommend.

wldcordeiro · 2017-08-04T01:59:12Z

So now the two assertions I added to the mochitests fail but from testing all three search inputs get focus when you open them. 🙁

Edit: Ran locally and they passed.

…mate it.

codehag reviewed Jul 31, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch from 48855cb to 0904bd0 Compare July 31, 2017 08:53

wldcordeiro commented Jul 31, 2017

View reviewed changes

jasonLaster reviewed Jul 31, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch from 0904bd0 to 6c7daac Compare July 31, 2017 17:19

jasonLaster reviewed Jul 31, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch from 6c7daac to 6a09000 Compare July 31, 2017 18:38

jasonLaster changed the base branch from next to master August 1, 2017 18:43

codehag force-pushed the fix-focus-issues branch from 6a09000 to e69461a Compare August 3, 2017 08:42

codehag reviewed Aug 3, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch 2 times, most recently from 800073e to f68c3fb Compare August 3, 2017 23:28

wldcordeiro commented Aug 3, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch 2 times, most recently from f7c39fe to 672a559 Compare August 4, 2017 01:18

wldcordeiro commented Aug 4, 2017

View reviewed changes

wldcordeiro force-pushed the fix-focus-issues branch 6 times, most recently from 3aa564f to 1e118e5 Compare August 7, 2017 15:59

wldcordeiro force-pushed the fix-focus-issues branch from 1e118e5 to cc6f822 Compare August 7, 2017 16:09

Fix the issues with inputs not receiving focus.

bf63bb4

wldcordeiro force-pushed the fix-focus-issues branch from cc6f822 to 6628741 Compare August 7, 2017 17:09

Use react-transition-group again to correctly mount the modal and ani…

2188a50

…mate it.

jasonLaster force-pushed the fix-focus-issues branch from 6628741 to 2188a50 Compare August 7, 2017 17:46

jasonLaster merged commit 4069f0f into firefox-devtools:master Aug 7, 2017

wldcordeiro deleted the fix-focus-issues branch August 7, 2017 17:55

Conversation

wldcordeiro commented Jul 31, 2017

Summary of Changes

Test Plan

Uh oh!

codecov Bot commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wldcordeiro commented Jul 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codehag commented Aug 3, 2017

Uh oh!

codehag commented Aug 3, 2017

Uh oh!

codehag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wldcordeiro commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

codecov Bot commented Jul 31, 2017 •

edited

Loading

wldcordeiro commented Aug 4, 2017 •

edited

Loading