Change mouse cursor when hovering/clicking shapes highlighter

RESOLVED FIXED in Firefox 58

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
26 days ago
9 days ago

People

(Reporter: mparkms, Assigned: mparkms)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [designer-tools])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

26 days ago
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 hidden (mozreview-request)

Comment 2

25 days ago
mozreview-review
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)
(Assignee)

Comment 3

25 days ago
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 hidden (mozreview-request)

Comment 6

18 days ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Whiteboard: [designer-tools]
(Assignee)

Updated

14 days ago
Keywords: checkin-needed

Comment 10

14 days ago
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
Comment hidden (mozreview-request)
(Assignee)

Updated

10 days ago
Keywords: checkin-needed

Comment 12

9 days ago
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

Comment 13

9 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e0bc9f7b713
Status: NEW → RESOLVED
Last Resolved: 9 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.