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

VERIFIED FIXED in Firefox 48

Status

P1
normal
VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: mboldan, Assigned: jsnajdr)

Tracking

47 Branch
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
[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

Updated

3 years ago
Priority: -- → P3
Whiteboard: [multiviewport] [triage] → [multiviewport]

Updated

3 years ago
Priority: P3 → P2
(Reporter)

Updated

3 years ago
status-firefox44: --- → unaffected
status-firefox45: --- → unaffected
status-firefox46: --- → unaffected

Updated

3 years ago
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]

Updated

3 years ago
Flags: qe-verify?

Updated

3 years ago
Flags: qe-verify? → qe-verify+
(Assignee)

Updated

3 years ago
Assignee: nobody → jsnajdr
(Assignee)

Comment 1

3 years ago
Created attachment 8741343 [details] [diff] [review]
The mouse cursor doesn't stick to the grippers while resizing the viewport.

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+
(Assignee)

Comment 4

3 years ago
Created attachment 8742911 [details] [diff] [review]
The mouse cursor doesn't stick to the grippers while resizing the viewport.

- 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)
(Assignee)

Updated

3 years ago
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

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21a6c80e744d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

3 years ago
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
(Reporter)

Comment 11

3 years ago
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
status-firefox49: --- → affected
Resolution: FIXED → ---
(Assignee)

Comment 12

3 years ago
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: → bug 1262839
(Assignee)

Comment 13

3 years ago
Created attachment 8747016 [details] [diff] [review]
responsive.html viewport correctly resized by mouse: fixed horizontal vs vertical, better test

- 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)
Blocks: 1262839
See Also: bug 1262839
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+
(Assignee)

Comment 16

3 years ago
Created attachment 8747410 [details] [diff] [review]
responsive.html viewport correctly resized by mouse: fixed horizontal vs vertical, better test

changed testViewPort -> testViewport as per jryans' comments.
Attachment #8747410 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8747016 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Checkin only the second patch that fixes the reopen. The first one has already landed.
Keywords: checkin-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70256f61e542
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
(Reporter)

Comment 20

3 years ago
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
status-firefox49: fixed → verified
Flags: qe-verify+
Whiteboard: [multiviewport] [mvp-rdm] → [multiviewport] [mvp-rdm] [qe-rdm]

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.