check "enable event handler" checkbox of AccessibleEvents viewer when user starts typing the event handler

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

2.23 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
to locate "enable event handler" checkbox and "event handler output" area go to AccessibleEvents viewer and then switch to WatchedEvents tab.

it'd be nice if you selected some event in the list, focused "event handler output" and started typing then that checkbox is checked automatically.
(Assignee)

Comment 1

6 years ago
Created attachment 595933 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #595933 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Summary: check "enable event target" checkbox of AccessibleEvents viewer when user starts typing the event handler → check "enable event handler" checkbox of AccessibleEvents viewer when user starts typing the event handler

Comment 2

6 years ago
Comment on attachment 595933 [details] [diff] [review]
patch

If you clear the contents of the handler, would you want to automatically disable it?

>-    var updateCheckbox = !updateData || aRowIdx == this.selection.currentIndex;
This breaks clicking on the checkboxes for other rows.

>+                   oninput="viewer.onWatchViewEditorChanged();"/>
Is it likely that the event could fire as the user was typing, and therefore incorrect code could be executed?
Attachment #595933 - Flags: review?(neil) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 596077 [details] [diff] [review]
patch2
Attachment #595933 - Attachment is obsolete: true
Attachment #596077 - Flags: review?(neil)

Comment 4

6 years ago
Comment on attachment 596077 [details] [diff] [review]
patch2

I like this approach :-)

>+    this.mHandlerEditor.addEventListener("blur", this, false);
Probably better to use "change" rather than "blur". r=me with that fixed.

>-    var idx = this.selection.currentIndex;
You end up using this.selection.currentIndex in both event types anyway...

>+    if (aEvent.type == "input") {
>+      var isEnabled = this.mHandlerEditor.value != "";
>+      this.updateHandlerState(isEnabled, this.selection.currentIndex);
[I was considering updating the handler state on the change event too, but I can't convince myself that it wouldn't be too confusing.]

>+      return;
Don't need this, type can't be both input and something else. (Or use switch.)
Attachment #596077 - Flags: review?(neil) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 596548 [details] [diff] [review]
patch3

addressed Neil's comments
Attachment #596077 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
landed http://hg.mozilla.org/dom-inspector/rev/9856b89c71a8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Blocks: 727380
Blocks: 670224
You need to log in before you can comment on or make changes to this bug.