Closed
Bug 1411402
Opened 7 years ago
Closed 7 years ago
Change mouse cursor when hovering/clicking shapes highlighter
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox58 verified, firefox61 verified, firefox62 verified)
VERIFIED
FIXED
Firefox 58
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 hidden (mozreview-request) |
Comment 2•7 years 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•7 years 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".
Comment 4•7 years ago
|
||
(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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years 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)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 12•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4e0bc9f7b713
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 14•6 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•