Closed Bug 1250138 Opened 8 years ago Closed 8 years ago

The mouse cursor doesn't stick to the grippers while resizing the viewport

Categories

(DevTools :: Responsive Design Mode, defect, P1)

47 Branch
defect

Tracking

(firefox44 unaffected, firefox45 unaffected, firefox46 unaffected, firefox47 affected, firefox48 fixed, firefox49 verified)

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- affected
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: mboldan, Assigned: jsnajdr)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm] [qe-rdm])

Attachments

(2 files, 2 obsolete files)

[Note]:
- Logged bug based on Bug 1241326 Comment 23 

[Affected versions]:
- Firefox 47.0a1 (2016-02-21)

[Affected platforms]:
- Windows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5

[Steps to reproduce]:
1. Launch Firefox.
2. From about:config, enable the devtools.responsive.html.enabled pref.
3. Open RDM.
4. Resize the viewport and observe the mouse cursor while the page is modified.

[Expected result]:
- The mouse cursor moves with the grippers.

[Actual result]:
- The mouse cursor moves faster than the grippers(see http://screencast.com/t/2e7ZrMV8y).

[Regression range]:
- This is not a regression since this is a new implementation.
Whiteboard: [multiviewport] → [multiviewport] [triage]
Summary: The mouse cursor doesn't stick to thee grippers while resizing the viewport → The mouse cursor doesn't stick to the grippers while resizing the viewport
Priority: -- → P3
Whiteboard: [multiviewport] [triage] → [multiviewport]
Priority: P3 → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Assignee: nobody → jsnajdr
When moving the mouse by X pixels, the viewport should be resized by 2X, because the viewport is centered.

Doesn't this need a mochitest and a try run? It would probably involve synthesizing a lot of mouse events or something like that. All responsive.html tests still pass locally.
Attachment #8741343 - Flags: review?(jryans)
Thanks for working on this, I'll take a look now.  Sorry for the delay!
Comment on attachment 8741343 [details] [diff] [review]
The mouse cursor doesn't stick to the grippers while resizing the viewport.

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

Thanks for working on this! :)

It would be great to have a mochitest to cover this as well, like you mentioned.  I'd suggest creating a new test and using the `addRDMTask` helper[1] to auto-open and auto-close the tool.  There is also a `waitForViewportResizeTo` helper[2] that may be useful.

Please free to ask on IRC, etc. if you have any questions about writing the test.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=addRDMTask&case=true
[2]: https://dxr.mozilla.org/mozilla-central/search?q=waitForViewportResizeTo&case=true

::: devtools/client/responsive.html/components/resizable-viewport.js
@@ +74,5 @@
>        return;
>      }
>  
>      let { lastClientX, lastClientY, ignoreX, ignoreY } = this.state;
> +    let deltaX = 2 * (clientX - lastClientX);

Add a comment above this line to explain why we need to multiply by 2.
Attachment #8741343 - Flags: review?(jryans) → feedback+
- added comment to explain why we are resizing by double amount
- added mochitest that test all kinds of resize (horizontal, vertical, full)
Attachment #8742911 - Flags: review?(jryans)
Attachment #8741343 - Attachment is obsolete: true
Comment on attachment 8742911 [details] [diff] [review]
The mouse cursor doesn't stick to the grippers while resizing the viewport.

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

Great, thanks for working on this!  The test looks well done to me. :)

Please feel free to look into other RDM bugs[1] that interest you!

[1]: http://mzl.la/1SAiq6f
Attachment #8742911 - Flags: review?(jryans) → review+
Filtered list just for RDM bugs:  http://mzl.la/1U5qugN
Status: NEW → ASSIGNED
(In reply to Marco Mucci [:MarcoM] from comment #7)
> Filtered list just for RDM bugs:  http://mzl.la/1U5qugN

The larger list I posted I believe includes the reserve list as well as MVP, so it's all stuff we'd like to have done, just different priority levels.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21a6c80e744d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
The issue is still reproducible on Firefox 49.0a1 (2016-04-28) if the viewport is resized in height (see http://screencast.com/t/eZKGuWuGsGAg ).  
The tests were performed on Windows 10 x86, Ubuntu 14.04 x86 and on Mac OS X 10.9.5.
I am reopening this issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Caused by changes from bug 1262839. Before that, the viewport was centered in both directions, now it's centered only horizontally.

The mochitest is not good enough to catch this. It should check the position of the resize handles after the drag.
See Also: → 1262839
- fixed the vertical resizing
- improved the test to verify the grippers position - next time change like bug 1262839 happens, it will fail
Attachment #8747016 - Flags: review?(jryans)
Comment on attachment 8747016 [details] [diff] [review]
responsive.html viewport correctly resized by mouse: fixed horizontal vs vertical, better test

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

Thanks for fixing this so fast! :)

::: devtools/client/responsive.html/test/browser/browser_mouse_resize.js
@@ +25,5 @@
> +
> +  return rect;
> +}
> +
> +function* testViewPortResize(ui, selector, moveBy,

Nit: Let's use "Viewport" instead of "ViewPort" to match other names
Attachment #8747016 - Flags: review?(jryans) → review+
changed testViewPort -> testViewport as per jryans' comments.
Attachment #8747410 - Flags: review+
Attachment #8747016 - Attachment is obsolete: true
Checkin only the second patch that fixes the reopen. The first one has already landed.
Keywords: checkin-needed
Attachment #8742911 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/70256f61e542
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
The bug was verified on Firefox 49.0a1 (2016-05-03) and on Mac OS X 10.10.5, Ubuntu 14.04 x86 and Windows 10 x64.
Since the issue is no longer reproducible, I am marking it Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [mvp-rdm] [qe-rdm]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.