Closed Bug 1411402 Opened 7 years ago Closed 7 years ago

Change mouse cursor when hovering/clicking shapes highlighter

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox58 verified, firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
firefox58 --- verified
firefox61 --- verified
firefox62 --- verified

People

(Reporter: mpark, Assigned: mpark)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(1 file)

The mouse cursor should change when hovering over or clicking on handles on the shapes highlighter, to indicate that mouse actions can be taken on the highlighter. Sensible choices would be "grab" when hovering over a point marker, "grabbing" when clicking and dragging it, "move" for the center point in transform mode, and "nesw-resize"/"nwse-resize" for the scaling handles in transform mode.
Comment on attachment 8922091 [details]
Bug 1411402 - Change mouse cursor when hovering/clicking shapes highlighter markers.

https://reviewboard.mozilla.org/r/193090/#review198500

Hmm, this looks complicated to me. Is there a really good reason not to use just CSS for this?
With CSS, something like this would work:
```cs
:-moz-native-anonymous .shapes-markers { pointer-events: auto; }
:-moz-native-anonymous .shapes-markers:hover { cursor: grab; }
```
Of course depending on which marker you are currently hovering over, different classes/attributes could be added to make it easy to select which cursor exactly we need to display.

This made me think, why do we have a hidden marker-hover element that is shown on hover? Would a CSS-only solution work here too?
Attachment #8922091 - Flags: review?(pbrosset)
Because we can't attach event handlers to individual elements in the highlighter, we determine if the mouse is hovering over a point by seeing if it's within 5 pixels of it. This creates a square hitbox that doesn't match perfectly with the circular markers. So if we were to use the :hover pseudo-class, the cursor change would not always accurately reflect whether you can interact with the point. This is also the reason we have the hidden marker-hover element instead of using CSS.

In addition, the cursor CSS property is applied to .shapes-root because if it were attached to .shapes-marker, the cursor change no longer applies if you are for example resizing the shape and your mouse goes off the point. Also, if you are dragging a polygon point, the marker plays catch up to your mouse position, resulting in an undesirable visual effect where the cursor rapidly flickers between "grab" and "auto".
(In reply to Mike Park [:mparkms] from comment #3)
> Because we can't attach event handlers to individual elements in the
> highlighter, we determine if the mouse is hovering over a point by seeing if
> it's within 5 pixels of it. This creates a square hitbox that doesn't match
> perfectly with the circular markers. So if we were to use the :hover
> pseudo-class, the cursor change would not always accurately reflect whether
> you can interact with the point. This is also the reason we have the hidden
> marker-hover element instead of using CSS.
> 
> In addition, the cursor CSS property is applied to .shapes-root because if
> it were attached to .shapes-marker, the cursor change no longer applies if
> you are for example resizing the shape and your mouse goes off the point.
> Also, if you are dragging a polygon point, the marker plays catch up to your
> mouse position, resulting in an undesirable visual effect where the cursor
> rapidly flickers between "grab" and "auto".

Thanks Mike for the detailed explanation. Can you please add this as a long comment in the code? Probably somewhere at the start of the file, so people in the future understand why we've done it this way, and don't attempt to move everything to CSS instead.
Comment on attachment 8922091 [details]
Bug 1411402 - Change mouse cursor when hovering/clicking shapes highlighter markers.

https://reviewboard.mozilla.org/r/193090/#review200972

::: devtools/server/actors/highlighters/shapes.js:1061
(Diff revision 2)
>    }
>  
>    _handleMarkerHover(point) {
>      // Hide hover marker for now, will be shown if point is a valid hover target
>      this.getElement("marker-hover").setAttribute("hidden", true);
> -    if (point === null || point === undefined) {
> +    if (point === null || point === undefined || point === "") {

Please add a jsdoc comment for this function so it's easy to understand why you're needing such a complex condition.

Also maybe something like that would be simpler:
`if (!Number.isInteger(point))`
Attachment #8922091 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2f0fd5343b4f -d f8903937a76e: rebasing 432319:2f0fd5343b4f "Bug 1411402 - Change mouse cursor when hovering/clicking shapes highlighter markers. r=pbro" (tip)
merging devtools/server/actors/highlighters/shapes.js
warning: conflicts while merging devtools/server/actors/highlighters/shapes.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e0bc9f7b713
Change mouse cursor when hovering/clicking shapes highlighter markers. r=pbro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4e0bc9f7b713
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Hi everyone, i have retested this issue on Windows 10 , Windows 7 , Mac OSx and Linux using the latest version of nightly 62.0a1 (2018-05-28) and i can Confirm it as Fixed.
I will mark it accordingly.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: