Rule view focus management needs an overhaul

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: miker, Assigned: dcamp)

Tracking

unspecified
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

v2
25.77 KB, patch
paul
: review+
Details | Diff | Splinter Review
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

6 years ago
Would it be possible to listen to "click", and stop the event if the selection start (onmousemove + selection non-empty)?

Updated

6 years ago
Blocks: 719832
(Assignee)

Comment 2

5 years ago
Created attachment 626640 [details] [diff] [review]
v1

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
Status: NEW → ASSIGNED
Attachment #626640 - Flags: review?(paul)
Attachment #626640 - Flags: feedback?(mratcliffe)
Comment on attachment 626640 [details] [diff] [review]
v1

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+

Comment 4

5 years ago
I see a cursor now: http://i.imgur.com/RnZEk.png
STR: click on a property name to bring the editor. Press ESC, then down.

Comment 5

5 years ago
Comment on attachment 626640 [details] [diff] [review]
v1

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+
(Assignee)

Comment 6

5 years ago
Created attachment 628491 [details] [diff] [review]
v2

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?
(Assignee)

Updated

5 years ago
Attachment #628491 - Flags: review? → review?(paul)
(Assignee)

Updated

5 years ago
Blocks: 745961

Comment 7

5 years ago
Comment on attachment 628491 [details] [diff] [review]
v2

It works. And it feels solid.
Attachment #628491 - Flags: review?(paul) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/85c153eea9d4
Whiteboard: [ruleview] → [ruleview][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/85c153eea9d4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
You need to log in before you can comment on or make changes to this bug.