[rep tests] Add tests for event rep

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Shared Components
P1
enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: linclark, Assigned: linclark)

Tracking

unspecified
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
See Bug 1257552
(Assignee)

Updated

2 years ago
Blocks: 1257552

Updated

2 years ago
Severity: normal → enhancement
Whiteboard: [devtools-html]

Updated

2 years ago
Blocks: 1263741

Updated

2 years ago
Flags: qe-verify-
Priority: -- → P2
(Assignee)

Comment 1

a year ago
It looks like only a few events have detailed information displayed: MouseEvent, KeyboardEvent, and MessageEvent. What was the reason for choosing those three?
Flags: needinfo?(odvarko)
(Assignee)

Updated

a year ago
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Iteration: --- → 50.2
Priority: P2 → P1
(Assignee)

Comment 2

a year ago
Created attachment 8764983 [details] [diff] [review]
Bug1264682.patch

I think we might want to handle events as an Object... that's what Chrome seems to do and it would come closer to the console's current output.
Attachment #8764983 - Flags: review?(odvarko)
(Assignee)

Comment 3

a year ago
Created attachment 8765000 [details] [diff] [review]
Bug1264682.patch
Attachment #8764983 - Attachment is obsolete: true
Attachment #8764983 - Flags: review?(odvarko)
Attachment #8765000 - Flags: review?(odvarko)
Comment on attachment 8765000 [details] [diff] [review]
Bug1264682.patch

Review of attachment 8765000 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should also cover the MessageEvent.

You can use window.postMessage("test data", "*") + window.addEventListener("message", msgEvent => {}, false);

Honza

::: devtools/client/shared/components/test/mochitest/test_reps_event.html
@@ +24,5 @@
> +    is(renderedRep.type, Event.rep, `Rep correctly selects ${Event.rep.displayName}`);
> +
> +    yield testEvent();
> +    yield testMouseEvent();
> +    yield testKeyboardEvent();

Can you please yet add: testMessageEvent();
Attachment #8765000 - Flags: review?(odvarko) → review-
(In reply to Lin Clark [:linclark] from comment #1)
> It looks like only a few events have detailed information displayed:
> MouseEvent, KeyboardEvent, and MessageEvent. What was the reason for
> choosing those three?
I don't recall, perhaps they have been requested by users. But, not sure what would be the other event types we'd like to support. In any case we should support all of them.

(In reply to Lin Clark [:linclark] from comment #2)
> I think we might want to handle events as an Object... that's what Chrome
> seems to do and it would come closer to the console's current output.

Nice thing about this specific support is that the user doesn't have to inspect the event object (click open/expand etc.) to see the most important values: i.e. coordinates for mouse event, key code for key event, data for the MessageEvent. They are visible immediately without any explicit user action.

In any case Event object should be clickable so, the user can inspect all props in VariablesView.

I vote for supporting this, but if "keeping the same UX" has higher priority now, we might want to use special mode for rendering back-compatible markup (I can't remember what was the suggested name for this mode?)

Honza
Flags: needinfo?(odvarko)
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
(Assignee)

Updated

a year ago
Blocks: 1284838
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
(Assignee)

Updated

a year ago
Attachment #8765000 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Created attachment 8773754 [details] [diff] [review]
test-event.patch

Honza's on PTO, so can you review, Mike?
Attachment #8773754 - Flags: review?(mratcliffe)
Attachment #8773754 - Flags: review?(mratcliffe) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 7

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b425256008f5
[rep tests] Add tests for event rep. r=mikeratcliffe
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b425256008f5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.