Image preview tooltip stack up in the markup view panel

VERIFIED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 823912 [details]
several panels (don't mind the colors, I've been playing with the panel CSS).png

STR:

- Go to a page that has several IMG tags next to each other (http://www.w3schools.com/tags/tag_img.asp works well, where the browser support logos are)
- Inspect that part of the DOM that contains the images
- Hover over the first IMG tag long enough so the tooltip appears
- Move your mouse down to the second IMG tag

-> Actual: The second tooltip appears and the first one remains.
-> Expected: The first tooltip should disappear as the mouse leaves the first IMG tag.
(Assignee)

Updated

4 years ago
Assignee: nobody → pbrosset
(Assignee)

Comment 1

4 years ago
Green try build: https://tbpl.mozilla.org/?tree=Try&rev=0c43046bf9f6
Attaching the patch...
(Assignee)

Comment 2

4 years ago
Created attachment 823992 [details] [diff] [review]
bug932218-several-image-tooltips-in-markup-view.patch
Attachment #823992 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

4 years ago
Oops, I was a bit hasty in saying the try build was green, there's been one orange on an unrelated test case.
I've relaunched a set of runs: https://tbpl.mozilla.org/?tree=Try&rev=0c43046bf9f6
Comment on attachment 823992 [details] [diff] [review]
bug932218-several-image-tooltips-in-markup-view.patch

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

This patch does fix the issue, but I think it could be done with less code.  I believe the first condition, with doShow, could be simplified to a single statement (given the default values for this._targetNodeCb).  Then, with only one other check for HOVER_MODE_SINGLE, I believe we could just store a boolean with a descriptive name on the object to keep track of this behavior.  Let me know if you think this reasoning is wrong, but if it could be simplified to not need the extra modes, I think it would be an improvement.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +227,5 @@
>  
>    _showOnHover: function(target) {
> +    let doShow = true;
> +    if (this._toggleOnHoverMode === HOVER_MODE_MULTI) {
> +      doShow = this._targetNodeCb && this._targetNodeCb(target, this);

It appears that you shouldn't need this if condition here.  If HOVER_MODE_SINGLE, then this._targetNodeCb is going to always return true (set in line 191).  If HOVER_MODE_MULTI, then it will be set to the user passed in function.  In this case, I believe that all you need is 

if (this._targetNodeCb(target, this))

@@ +239,5 @@
>  
>    _onBaseNodeMouseLeave: function() {
>      clearNamedTimeout(this.uid);
>      this._lastHovered = null;
> +    if (this._toggleOnHoverMode === HOVER_MODE_SINGLE) {

Since this is the only place the const would be checked (if the condition in my comment above is equivalent), maybe we just need to store a boolean on the object, maybe like this._hideOnMouseLeave = !!targetNodeCb.
Attachment #823992 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

4 years ago
This is exactly what I set out to do in the first place but decided against it after a while, thinking that the explicit MODES would make things a little bit easier to understand.
But I'm happy to revert that to less code and just the this._hideOnMouseLeave boolean.
Thanks. I'll upload a new patch soon.
(Assignee)

Comment 6

4 years ago
Created attachment 824065 [details] [diff] [review]
bug932218-several-image-tooltips-in-markup-view V2.patch

Simplifies the code a bit as per bgrins' review.
Attachment #823992 - Attachment is obsolete: true
Attachment #824065 - Flags: review?(bgrinstead)
Attachment #824065 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 7

4 years ago
Thanks Brian for the review!
(Assignee)

Comment 8

4 years ago
Try build for the v2 patch is green: https://tbpl.mozilla.org/?tree=Try&rev=459ec6554c9f
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3de88d67b2de
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3de88d67b2de
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/3de88d67b2de

Updated

4 years ago
Keywords: verifyme

Comment 12

4 years ago
Reproduced with Nightly from 2013-10-29.
Verified as fixed on latest Aurora (Build ID:20140203004003): the selected tooltip disappears when I move the mouse over another img tag. If I don't move the mouse cursor and just scroll up/down to another img tag, it doesn't dissapear nor change; on Chrome and Opera it's the opposite. Is it the intended behavior?
Flags: needinfo?(pbrosset)
(Assignee)

Comment 13

4 years ago
No it is an intended behavior, but there's a separate bug filed for this: bug 938097
Flags: needinfo?(pbrosset)

Updated

4 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.