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)
DevTools
Inspector
Tracking
(firefox61- verified)
VERIFIED
FIXED
Firefox 61
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.
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
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)
Updated•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
tracking-firefox61:
--- → +
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → rcaliman
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 7•6 years ago
|
||
Please set r+ on the left in MozReview so MozReview can land that.
Flags: needinfo?(gl)
Comment 8•6 years ago
|
||
mozreview-review |
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
Updated•6 years ago
|
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
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
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+
Assignee | ||
Comment 14•6 years ago
|
||
Latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d55054804eec5a585dc38f05478f95f38efea8c
Keywords: checkin-needed
Comment 15•6 years ago
|
||
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
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb5f1d08489f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 17•6 years ago
|
||
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)
Assignee | ||
Comment 18•6 years ago
|
||
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 → ---
Comment 19•6 years ago
|
||
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)
Assignee | ||
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
Please Note that the same issue occurs for "Inset" shape as well.
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Reminder for me to test the "dragging under scrollbar" problem on Windows.
Flags: needinfo?(pbrosset)
Comment 24•6 years ago
|
||
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 25•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•6 years ago
|
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•6 years ago
|
||
Opened follow-up bug 1457838 with the latest patch which Patrick reviewed. Closing this back to its previous status resolved/fixed.
Updated•6 years ago
|
Flags: needinfo?(pbrosset)
Comment 27•6 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•