Last Comment Bug 734365 - Rule view focus management needs an overhaul
: Rule view focus management needs an overhaul
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
P2 normal (vote)
: Firefox 15
Assigned To: Dave Camp (:dcamp)
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Depends on:
Blocks: 719832 745961
  Show dependency treegraph
Reported: 2012-03-09 05:51 PST by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-06-02 02:28 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (19.61 KB, patch)
2012-05-23 17:13 PDT, Dave Camp (:dcamp)
paul: review+
mratcliffe: feedback+
Details | Diff | Splinter Review
v2 (25.77 KB, patch)
2012-05-30 14:55 PDT, Dave Camp (:dcamp)
paul: review+
Details | Diff | Splinter Review

Description User image Michael Ratcliffe [:miker] [:mratcliffe] 2012-03-09 05:51:27 PST
The rule view currently triggers it's editor whenever e.g. a name or value node receives focus. This method has it's shortcomings as evidenced by mousing down to start selecting text.

We should change this so that the property editor is not triggered when mousing down to select text. Not sure how this is best done ... maybe using mouseup instead of focus to trigger editing?
Comment 1 User image Paul Rouget [:paul] 2012-03-09 06:07:24 PST
Would it be possible to listen to "click", and stop the event if the selection start (onmousemove + selection non-empty)?
Comment 2 User image Dave Camp (:dcamp) 2012-05-23 17:13:12 PDT
Created attachment 626640 [details] [diff] [review]

The attached patch makes a few changes:

a) Editors are only started on click, not focus events.  We still use focus management to move between fields, but initial triggering of the editor is only done with a click.  This keeps keynav working well.

b) The editor explicitly takes care of its commit handler etc. while handling events.  Before this patch it would cause blurs and rely on blur handlers to apply things asynchronously.

c) As a result of these changes, we were able to get rid of a few hacks (_selectionMode and the focus backstop crud).

d) Selection now works from anywhere (before this patch, you can't start a selection inside a property name).
Comment 3 User image Michael Ratcliffe [:miker] [:mratcliffe] 2012-05-24 03:22:55 PDT
Comment on attachment 626640 [details] [diff] [review]

Awesome, especially because it gets rid of a gazillion ugly hacks at the same time as making the editor more accessible and maintainable.

100% f+ from me.
Comment 4 User image Paul Rouget [:paul] 2012-05-25 12:47:02 PDT
I see a cursor now:
STR: click on a property name to bring the editor. Press ESC, then down.
Comment 5 User image Paul Rouget [:paul] 2012-05-25 14:04:37 PDT
Comment on attachment 626640 [details] [diff] [review]

Review of attachment 626640 [details] [diff] [review]:

About the "ugly workaround". I am not sure this is really useful, as the dotted line is visible just for a very short instant (but I'm ok with this).

r+ if you can fix the cursor.
Comment 6 User image Dave Camp (:dcamp) 2012-05-30 14:55:36 PDT
Created attachment 628491 [details] [diff] [review]

This patch changed just enough that I'm going to reask for review:

* Fixed the caret problem by focusing the span after the editor is cancelled.
* Refactored some tests a little bit.
Comment 7 User image Paul Rouget [:paul] 2012-06-01 09:36:12 PDT
Comment on attachment 628491 [details] [diff] [review]

It works. And it feels solid.
Comment 9 User image Panos Astithas [:past] 2012-06-02 02:28:30 PDT

Note You need to log in before you can comment on or make changes to this bug.