Closed
Bug 1349416
Opened 7 years ago
Closed 6 years ago
Clicking on event listener doesn't close the event listener tooltip
Categories
(DevTools :: Inspector, defect, P3)
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)
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
mantaroh
:
feedback+
birtles
:
feedback+
|
Details |
46 bytes,
text/x-phabricator-request
|
birtles
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
birtles
:
review+
|
Details | Review |
46 bytes,
text/x-phabricator-request
|
birtles
:
review+
|
Details | Review |
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.
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
OS: Unspecified → All
Hardware: Unspecified → All
Comment 2•7 years ago
|
||
Marking fix-optional for 55 as this is P3.
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•6 years ago
|
status-firefox52:
unaffected → ---
status-firefox53:
unaffected → ---
status-firefox54:
unaffected → ---
status-firefox55:
fix-optional → ---
See Also: → 1473209
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Attachment #8995417 -
Flags: feedback?(mantaroh)
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
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
Assignee | ||
Comment 11•6 years ago
|
||
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•6 years ago
|
||
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 13•6 years ago
|
||
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+
Comment 14•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4f8eccf14dc Hide HTMLTooltip on mouseup rather than on click;r=birtles
Comment 15•6 years ago
|
||
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+
Comment 16•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ec5ee2990e21 Only delay removeEventListener when hiding after mouseup;r=birtles
Comment 17•6 years ago
|
||
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+
Comment 18•6 years ago
|
||
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
Assignee | ||
Comment 19•6 years ago
|
||
Rebased and fixed the issue. New try push based on latest central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87935dcb45606cdaca89cfea9bb73cc953e24a5
Assignee | ||
Comment 20•6 years ago
|
||
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•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c514bf46e52b Prevent event tooltip from reappearing when clicking on bubble;r=birtles
Comment 22•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4578670771f Hide HTMLTooltip on mouseup rather than on click;r=birtles
Comment 23•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/61021fadb55e Only delay removeEventListener when hiding after mouseup;r=birtles
Comment 24•6 years ago
|
||
Yeah, I've just been pushing to inbound until Lando supports more than one patch at a time.
Assignee | ||
Comment 25•6 years ago
|
||
(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•6 years ago
|
||
bugherder |
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: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 27•6 years ago
|
||
(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•6 years ago
|
||
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.
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → wontfix
Flags: in-testsuite+
Assignee | ||
Comment 29•6 years ago
|
||
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.
Description
•