Closed Bug 1450650 Opened 6 years ago Closed 6 years ago

The points of the DevTools Shape Tool can be dragged outside the page

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox61- verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 - verified

People

(Reporter: david.olah, Assigned: rcaliman)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

[Affected versions]: 
Nightly 61.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Windows 7 x 32, Mac OS X 10.12 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Go to about:config and set "layout.css.shape-outside.enabled" to true.
2. Go to http://labs.jensimmons.com/2016/examples/shapes-3.html
3. Right click on the image and then click on "Inspect Element".
4. Click on the button displayed next to clip-path to toggle the shape tool on
5. Drag a point outside the page (Ex: under the Firefox top bar)

[Expected result]:
- The point stops at the borders of the web page and cannot be dragged outside of it

[Actual result]:
- The point goes outside the page and cannot be moved back.
Not being able to move back is indeed problematic, but at the same, preventing users from moving points outside the viewport feels a bit restrictive. There are probably not going to be many times when users will actually want to do that, but when they do, it will be a problem for them if they can't do it with the tool.

For stylistic reasons, you may very well want a shape to extend past the viewport, but at the same time, not being able to grab a point that has been moved outside of the viewport means that you wouldn't use the tool for these cases anyway. So, should the tool prevent you from doing it? Or just allow you to do anything that is correct in CSS, even if it puts you in a bad spot later?

A question for Martin probably.
Flags: needinfo?(mbalfanz)
I think it's a good bug to keep in mind, but I don't see this as blocking for release.

As described my Patrick, I would not restrict the tool if the selected values are allowed by the spec. At the same time the user might run into weird situations, having trouble to get out of it again. We should think about ways to support the user here in the future, e.g. by providing indicators at the viewport edges that you can use to get control over the point back.
Flags: needinfo?(mbalfanz)
Priority: -- → P3
Actually, not tracking for 61 given comment 2.
Assignee: nobody → rcaliman
I added a patch which constrains the dragged marker to the current viewport. This prevents authors from accidentally distorting their shapes in such a way that's not immediately obvious how to recover from. 

This clamps dragging of:
- polygon points
- ellipse center and radii
- circle center and radius (for radius, only when dragging horizontally, not diagonally)
- inset edges

Authors can always edit the text of the shape value in the Rule view to recover a point which is out of the viewport. 

Shapes with coordinates which exceed the element / viewport boundaries are considered valid as defined by the CSS Shapes spec. For stylistic reasons, they may even be desired by the author. The tool will not attempt to correct such shape values when it encounters them. It only prevents dragging of individual coordinates out of the viewport.

Turning on transforms mode (Ctrl+click/Cmd+click) and dragging/scaling a shape in such a way that some of its coordinates go outside the viewport will not cause those coordinates to be constrained to the viewport. This would distort the shape and, most likely, it is not the desired effect the author wants.
Comment on attachment 8967731 [details]
Bug 1450650 - Shape editor: account for scrollbars when dragging markers to the edges of the viewport.

https://reviewboard.mozilla.org/r/236448/#review242508
Attachment #8967731 - Flags: review?(gl) → review+
Keywords: checkin-needed
Please set r+ on the left in MozReview so MozReview can land that.
Flags: needinfo?(gl)
Comment on attachment 8967731 [details]
Bug 1450650 - Shape editor: account for scrollbars when dragging markers to the edges of the viewport.

https://reviewboard.mozilla.org/r/236448/#review243048
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab522801015
Clamp x/y values to current viewport when dragging a marker in the shape editor. r=gl
Keywords: checkin-needed
Backed out changeset 3ab522801015 (bug 1450650) for mochitest devtools failures on browser_inspector_highlighter-cssshape_iframe_01.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6293a56d3b0d7ebf428bed8c8fd4dc7697d5718e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3ab522801015f2ff098f9a7958f8348a862955bf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=174131537

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174131537&repo=mozilla-inbound&lineNumber=2744

:19     INFO -  451 INFO TEST-START | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_07.js
16:18:23     INFO -  GECKO(3456) | MEMORY STAT | vsize 2099048MB | vsizeMaxContiguous 128994909MB | residentFast 340MB | heapAllocated 140MB
16:18:23     INFO -  452 INFO TEST-OK | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_07.js | took 3889ms
16:18:23     INFO -  453 INFO checking window state
16:18:23     INFO -  454 INFO TEST-START | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js
16:18:24     INFO -  TEST-INFO | started process screenshot
16:18:24     INFO -  TEST-INFO | screenshot: exit 0
16:18:24     INFO -  Buffered messages logged at 16:18:23
16:18:24     INFO -  455 INFO Entering test bound
16:18:24     INFO -  456 INFO Adding a new tab with URL: http://example.com/browser/devtools/client/inspector/test/doc_inspector_highlighter_cssshapes_iframe.html
16:18:24     INFO -  457 INFO Tab added and finished loading
16:18:24     INFO -  458 INFO Opening the inspector
16:18:24     INFO -  459 INFO Opening the toolbox
16:18:24     INFO -  Buffered messages logged at 16:18:24
16:18:24     INFO -  460 INFO Toolbox opened and focused
16:18:24     INFO -  461 INFO Turn on shapes highlighter for #polygon
16:18:24     INFO -  462 INFO Selecting the node for '[Front for domnode/server1.conn37.child1/domnode43]'
16:18:24     INFO -  463 INFO Toggle shapes highlighter on for clip-path on #polygon
16:18:24     INFO -  464 INFO Moving polygon point visible in iframe
16:18:24     INFO -  Buffered messages finished
16:18:24    ERROR -  465 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js | Point moved to 10px 10px -
16:18:24     INFO -  Stack trace:
16:18:24     INFO -  chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js:testPolygonIframeMovePoint:46
16:18:24     INFO -  466 INFO Moving polygon point not visible in iframe
16:18:24     INFO -  467 INFO TEST-PASS | devtools/client/inspector/test/browser_inspector_highlighter-cssshape_iframe_01.js | Point moved to 110px 51.25% -
16:18:24     INFO -  468 INFO Leaving test bound
16:18:25     INFO -  469 INFO Removing tab.
16:18:25     INFO -  470 INFO Waiting for event: 'TabClose' on [object XULElement].
16:18:25     INFO -  471 INFO Got event: 'TabClose' on [object XULElement].
16:18:25     INFO -  472 INFO Tab removed and finished closing
Flags: needinfo?(rcaliman)
This patch needs to go in after the patch for Bug 1453428 has landed. That patch has the fix for the mochitest which is affecting this one too.

I'll wait for that to land, rebase my changes and submit an updated patch.
Flags: needinfo?(rcaliman)
Forwarding r+ from previous patch with @gl's approval.

This patch introduces fixes for handling shapes within iframes. The mochitest which caused the back-out was failing because this case wasn't handled. 

Markers for shapes inside iframes will be clamped to the iframe viewport. If the markers are drawn outside the iframe's viewport, but visible in the context of the iframe's parent, they won't be clamped unless they're brought within the iframe viewport, released, then dragged again.
Attachment #8967731 - Attachment is obsolete: true
Attachment #8968995 - Flags: review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5f1d08489f
Clamp x/y values to current viewport when dragging a marker in the shape editor. r=gl
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb5f1d08489f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Hello, 

After testing this issue again , the highlighter points stop on the top / bottom and left margin, but on the right side of the screen the Highlighter point will be hidden under the Scroll bar and the user will be unable to move it back.

Please see the Attached Screnshot.
Flags: needinfo?(rcaliman)
You're right. I have not tested with scrollbars on because I develop on a Mac with hidden scrollbars.
I'll look into a fix which accounts for scrollbars in the viewport.
Status: RESOLVED → REOPENED
Flags: needinfo?(rcaliman)
Resolution: FIXED → ---
Hi Razvan, Please note that the same issue still occurs on Ellipse and Circle shapes, you can check the issue on the following links, let me know if there should be a separate defect added for each and i will log them afterwards. 

http://labs.jensimmons.com/2016/examples/shapes-2.html - Ellipse

http://labs.jensimmons.com/2016/examples/shapes-1.html - Circle
Flags: needinfo?(rcaliman)
Yes, dragging any marker under the scrollbar is a bug that affects all shape types. It's a bug with how viewport constraints are calculated. No need to log separate issues.
Flags: needinfo?(rcaliman)
Please Note that the same issue occurs for "Inset" shape as well.
Reminder for me to test the "dragging under scrollbar" problem on Windows.
Flags: needinfo?(pbrosset)
Note that mozreview seems a little bit confused here. It shows me an interdiff with what has landed already.
I suggest leaving this bug resolved/fixed and adding this additional fix in a separate bug.
Comment on attachment 8967731 [details]
Bug 1450650 - Shape editor: account for scrollbars when dragging markers to the edges of the viewport.

https://reviewboard.mozilla.org/r/236448/#review246348

Code changes look good to me, and I tested on Windows 10, the issue is addressed.
Attachment #8967731 - Flags: review?(pbrosset) → review+
See Also: → 1457838
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Opened follow-up bug 1457838 with the latest patch which Patrick reviewed. Closing this back to its previous status resolved/fixed.
Flags: needinfo?(pbrosset)
Hello, i have retested this issue on Windows 10 , Mac OS X and Ubuntu using the latest version of nightly 61.0a1 (2018-05-03) and it no longer reproduces.

I can Confirm it as fixed and mark it as Verified.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.