1349416 - Clicking on event listener doesn't close the event listener tooltip

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[684sigma]

I noticed a problem in Nightly 55. It doesn't happen in Beta 52, Beta 53.
Clicking on the same event listener doesn't close the event listener tooltip.
Here's what I do

1. Open attachment 8843613 [details]
2. Open inspector
3. Click on event listener 1
4. Click on event listener 1

Result: tooltip for event listener 1 blinks
Expected: tooltip for event listener 1 should hide

I think Bug 1344504 could cause this.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Julien Cristau [:jcristau] (back August 18)]

Marking fix-optional for 55 as this is P3.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1349416 - Prevent event tooltip from reappearing when clicking on bubble

Review commit: https://reviewboard.mozilla.org/r/259864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259864/

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Review commit: https://reviewboard.mozilla.org/r/259866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259866/

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8995417 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Mantaroh, Brian, since you both lately worked on the tooltip I'd love to have your opinion on the change here. Relying on the "click" event to hide the tooltip is not good enough for the event tooltip.

I think the reason for that is that the markupview is regularly recreating elements on mousedown since it uses a templating approach. Consequently, when clicking we often have a different element on mouseup compared to the one we had on mousedown. And no "click" event gets fired. So I would like to move the "hiding" logic from the "click" handler to a "mouseup" handler.

Note that I added a timeout here in order to keep the consumeOutsideClicks feature working.

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8995417 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Looks good to me.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8995417 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Thanks, Julian.

It looks good to me too.

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the feedback! This obviously needs a bit of polish (and makes a bunch of racy mochitests fail :)) so I won't submit to review straight away.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1349416 - Prevent event tooltip from reappearing when clicking on bubble;r=birtles

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click;r=birtles

A click event is only fired if the element on which mousedown and mouseup
happened are the same. If this is not the case, the HTMLTooltip will not
be able to hide itself.

Listening to mouseup makes the behavior more consistent but forces the
hide() method to be always asynchronous, while before it was only
asynchronous for tooltips using the XULWrapper.

Depends on D2660

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1349416 - Only delay removeEventListener when hiding after mouseup;r=birtles

The inplace-editor is programmatically hiding the autocomplete-popup
on TAB and notifies the caller. The caller (eg rule view) will trigger
a click on the next focusable element immediately after that.

If the HTMLTooltip is still consuming outside clicks at that point
the click will be swallowed and the new inplace-editor cannot be created.

Depends on D2661

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7e80d3a6810
Prevent event tooltip from reappearing when clicking on bubble;r=birtles

Comment 13

[Phabricator Automation]

Comment on attachment 8997291 [details]
Bug 1349416 - Prevent event tooltip from reappearing when clicking on bubble;r=birtles

Brian Birtles (:birtles) has approved the revision.

https://phabricator.services.mozilla.com/D2660

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4f8eccf14dc
Hide HTMLTooltip on mouseup rather than on click;r=birtles

Comment 15

[Phabricator Automation]

Comment on attachment 8997292 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click;r=birtles

Brian Birtles (:birtles) has approved the revision.

https://phabricator.services.mozilla.com/D2661

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec5ee2990e21
Only delay removeEventListener when hiding after mouseup;r=birtles

Comment 17

[Phabricator Automation]

Comment on attachment 8997293 [details]
Bug 1349416 - Only delay removeEventListener when hiding after mouseup;r=birtles

Brian Birtles (:birtles) has approved the revision.

https://phabricator.services.mozilla.com/D2662

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 3 changesets (Bug 1349416) for Dev Tools failures.

Pushes with failures (3 patches on 3 change sets):
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b7e80d3a6810f1b98fc0703a0772b6657dfb1ed9
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c4f8eccf14dcdc14dee9aa597c8a42ed31c75555
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ec5ee2990e219b1cd1c95053a21878d69ddad52c

Backout link: https://hg.mozilla.org/integration/autoland/rev/fe0afb18004246b75c6b5586816dfa9390010379

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=191816087&repo=autoland&lineNumber=3525

Comment 19

[Julian Descottes [:jdescottes]]

Rebased and fixed the issue. New try push based on latest central:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87935dcb45606cdaca89cfea9bb73cc953e24a5

Comment 20

[Julian Descottes [:jdescottes]]

New try seems green, will land when autoland reopens. As before please only look at test results for the last changeset, as all 3 changesets are needed to get all tests to pass.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c514bf46e52b
Prevent event tooltip from reappearing when clicking on bubble;r=birtles

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4578670771f
Hide HTMLTooltip on mouseup rather than on click;r=birtles

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61021fadb55e
Only delay removeEventListener when hiding after mouseup;r=birtles

Comment 24

[Brian Birtles (:birtles)]

Yeah, I've just been pushing to inbound until Lando supports more than one patch at a time.

Comment 25

[Julian Descottes [:jdescottes]]

(In reply to Brian Birtles (:birtles) from comment #24)
> Yeah, I've just been pushing to inbound until Lando supports more than one
> patch at a time.

When you are pushing to inbound, do you need to manually close the phabricator reviews, or is it done automatically?

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c514bf46e52b
https://hg.mozilla.org/mozilla-central/rev/c4578670771f
https://hg.mozilla.org/mozilla-central/rev/61021fadb55e

Comment 27

[Brian Birtles (:birtles)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #25)
> (In reply to Brian Birtles (:birtles) from comment #24)
> > Yeah, I've just been pushing to inbound until Lando supports more than one
> > patch at a time.
> 
> When you are pushing to inbound, do you need to manually close the
> phabricator reviews, or is it done automatically?

It's a manual process I'm afraid. "Close Revision" from the actions list.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

IMO, this can ride the trains given what an old regression it is. Feel free to nominate for Beta approval if you feel strongly otherwise.

Comment 29

[Julian Descottes [:jdescottes]]

Happy to let this ride the trains, could use some baking time on Nightly anyway.