Closed Bug 1349416 Opened 5 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector, defect, P3)

55 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: 684sigma, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(5 files)

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.
Has STR: --- → yes
Keywords: regression
See Also: → 1344504
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Blocks: 1344504
OS: Unspecified → All
Hardware: Unspecified → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Marking fix-optional for 55 as this is P3.
Product: Firefox → DevTools
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8995417 - Flags: feedback?(mantaroh)
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.
Attachment #8995417 - Flags: feedback?(bbirtles)
Comment on attachment 8995417 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Looks good to me.
Attachment #8995417 - Flags: feedback?(bbirtles) → feedback+
Comment on attachment 8995417 [details]
Bug 1349416 - Hide HTMLTooltip on mouseup rather than on click

Thanks, Julian.

It looks good to me too.
Attachment #8995417 - Flags: feedback?(mantaroh) → feedback+
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.
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
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
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7e80d3a6810
Prevent event tooltip from reappearing when clicking on bubble;r=birtles
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
Attachment #8997291 - Flags: review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4f8eccf14dc
Hide HTMLTooltip on mouseup rather than on click;r=birtles
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
Attachment #8997292 - Flags: review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec5ee2990e21
Only delay removeEventListener when hiding after mouseup;r=birtles
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
Attachment #8997293 - Flags: review+
Rebased and fixed the issue. New try push based on latest central:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87935dcb45606cdaca89cfea9bb73cc953e24a5
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.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c514bf46e52b
Prevent event tooltip from reappearing when clicking on bubble;r=birtles
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4578670771f
Hide HTMLTooltip on mouseup rather than on click;r=birtles
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61021fadb55e
Only delay removeEventListener when hiding after mouseup;r=birtles
Yeah, I've just been pushing to inbound until Lando supports more than one patch at a time.
(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?
https://hg.mozilla.org/mozilla-central/rev/c514bf46e52b
https://hg.mozilla.org/mozilla-central/rev/c4578670771f
https://hg.mozilla.org/mozilla-central/rev/61021fadb55e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(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.
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.
Happy to let this ride the trains, could use some baking time on Nightly anyway.
You need to log in before you can comment on or make changes to this bug.