970172 - VariablesView doesn't correctly handle clicking while input is active

Status: VERIFIED FIXED

(DevTools :: Object Inspector, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Open Network Ctrl+Shift+Q
2. Open any web page
3. Select a request GET/PPOST
4. Select a header value of Request/Response(e.g. Referer)
   And Right click on value
5. Choose "Copy"

Actual Results:
Nothing is copied 

Expected Results:
Value should be copied

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/4bf430d990e5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203071739
Bad:
http://hg.mozilla.org/mozilla-central/rev/85694fd9b17c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203134611
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4bf430d990e5&tochange=85694fd9b17c

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/38ddf36afa9d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203131247
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8dfe4e73db8e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203144254
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=38ddf36afa9d&tochange=8dfe4e73db8e


Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/7b8cc2b3568b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203094749
Bad:
http://hg.mozilla.org/integration/fx-team/rev/e0572cb4eb82
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131203101549
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=7b8cc2b3568b&tochange=e0572cb4eb82

Regressed by
e0572cb4eb82	Brandon Benvie — Bug 808371 - Allow adding new properties to objects in the VariablesView. r=vp, r=jryans

Comment 2

[Victor Porof [:vporof][:vp]]

Thanks for digging that out. Bug 808371 looks like the one changeset that makes most sense in that list.

Comment 3

[Brandon Benvie [:benvie]]

Looks like mousedown on the menu is causing it to close, so the click action (copy) never runs. Probably related to the changes from bug 808371 as victor said.

Comment 4

[Brandon Benvie [:benvie]]

Attachment: variables-view-context.patch

The problem was that we no longer check whether an input is active. Previously, we could detect whether we were editing by checking whether `variable._inputNode`. With bug 808371 that was no longer checked. This patch adds an "editing" property that serves this purpose.

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8373794 [details] [diff] [review]
variables-view-context.patch

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

This looks perfectly good to me. Would it be possible to write a test for it?

Comment 7

[Brandon Benvie [:benvie]]

That's the plan! Bug 970961 has a better failure mode for testing fortunately. Dealing with context menus is icky.

Comment 8

[Brandon Benvie [:benvie]]

Attachment: variables-view-context.patch

Adds a test. The test uses watch expression editing to confirm the behavior, but it also covers the contextmenu case (since the contextmenu counts as a child of the textbox, clicking on it has the same behavior as clicking on the textbox itself for our purposes).

https://tbpl.mozilla.org/?tree=Try&rev=b7c9c40905b7

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 8374436 [details] [diff] [review]
variables-view-context.patch

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

Some oranges here.

Comment 10

[Brandon Benvie [:benvie]]

The use of EventUtils.synthesizeMouse in the new test appears to cause the failures in other completely unrelated tests.

Comment 11

[Brandon Benvie [:benvie]]

Attachment: variables-view-context.patch

Nothing I've tried besides synthesizeMouse actually triggers the behavior that needs to be tested here, as confirmed by the `input.selectionStart === input.selectionEnd` assertion.

With a vague handwavy thought process of "grumble grumble focus? something?", I added another synthesizeMouse click on the VariablesView container at the end of the test. This magically fixes the other test failures, but I don't understand why it works (or why it's needed).

https://tbpl.mozilla.org/?tree=Try&rev=788a2b24d4ac

Comment 12

[Victor Porof [:vporof][:vp]]

:/

Comment 13

[Brandon Benvie [:benvie]]

I haven't given up on figuring it out yet! I do want to see if this fixes it on try though. In the previous try run, different platforms each failed at slightly different points. Locally, I was only able to reproduce failures for one of the tests.

Comment 14

[Brandon Benvie [:benvie]]

Comment on attachment 8375166 [details] [diff] [review]
variables-view-context.patch

I've spent some time on this but haven't really come up with a good explanation of what the issue is here and why the extra synthesizeMouse fixes it. Do you have any ideas on how to debug this? If not, do you think we can land this as is? It fixes the bug and the behavior is tested...

Comment 15

[Victor Porof [:vporof][:vp]]

Comment on attachment 8375166 [details] [diff] [review]
variables-view-context.patch

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

#yolo

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2083,5 @@
>    delete: null,
>    new: null,
>    preventDisableOnChange: false,
>    preventDescriptorModifiers: false,
> +  editing: false,

Might want to make this a private property with a corresponding isEditing getter.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

We're building our final FF28 beta today so this is going to miss that release.  Please get in touch with release management if there's a serious potential user impact here, otherwise nominate any low-risk fix to branch uplift when ready.

Comment 17

[Victor Porof [:vporof][:vp]]

Benvie, are you going to land this?

Comment 18

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/7ea0df19bc37

Comment 19

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vporof][:vp] from comment #17)
> Benvie, are you going to land this?

Totally fell off my radar... for reason I had it marked as landed.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ea0df19bc37

Comment 21

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-02-10, Win 7 x64.
Verified fixed 31.0a1 (2014-03-20).

Comment 22

[Rob Campbell [:rc] (:robcee)]

I see this is tracking both beta and australis. Should we get it uplifted?

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8375166 [details] [diff] [review]
variables-view-context.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature
User impact if declined: clicking won't work correctly in variablesview
Testing completed (on m-c, etc.):  m-c
Risk to taking this patch (and alternatives if risky): minimal risk.
String or IDL/UUID changes made by this patch: none.

Comment 24

[Rob Campbell [:rc] (:robcee)]

s/australis/aurora/

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c81e196a69d4
https://hg.mozilla.org/releases/mozilla-beta/rev/43bf8604d459