Closed Bug 734365 Opened 9 years ago Closed 9 years ago

Rule view focus management needs an overhaul


(DevTools :: Inspector, defect, P2)



(Not tracked)

Firefox 15


(Reporter: miker, Assigned: dcamp)



(Whiteboard: [ruleview][fixed-in-fx-team])


(1 file, 1 obsolete file)

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?
Would it be possible to listen to "click", and stop the event if the selection start (onmousemove + selection non-empty)?
Blocks: 719832
Attached patch v1 (obsolete) — Splinter 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).
Assignee: nobody → dcamp
Attachment #626640 - Flags: review?(paul)
Attachment #626640 - Flags: feedback?(mratcliffe)
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.
Attachment #626640 - Flags: feedback?(mratcliffe) → feedback+
I see a cursor now:
STR: click on a property name to bring the editor. Press ESC, then down.
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.
Attachment #626640 - Flags: review?(paul) → review+
Attached patch v2Splinter 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.
Attachment #626640 - Attachment is obsolete: true
Attachment #628491 - Flags: review?
Attachment #628491 - Flags: review? → review?(paul)
Blocks: 745961
Comment on attachment 628491 [details] [diff] [review]

It works. And it feels solid.
Attachment #628491 - Flags: review?(paul) → review+
Whiteboard: [ruleview] → [ruleview][fixed-in-fx-team]
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.