Closed Bug 1455588 Opened 3 years ago Closed 3 years ago

The User is unable to Move the Highlighter Points outside of an iFrame

Categories

(DevTools :: Inspector, defect)

All
Unspecified
defect
Not set
normal

Tracking

(firefox61 verified)

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified

People

(Reporter: rares.doghi, Assigned: rcaliman)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached image iFrameBoundaries.png
[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 "https://drive.google.com/file/d/1zAOwoVHBDk8RmX97N7lvvdbd5KPWRk6u/view?usp=sharing" downloaded it in .html formate and open it with Firefox, Make sure the element is inside the iframe.
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, and make sure the page is iframed.
5. Drag the points outside of the iFrame in order to change the shape.

[Expected result]:
- The User should be able to move the Points past the iFrame but the Points should still be constrained to the Parent Window boundaries.

[Actual result]:
- The highlighter points are locked inside the iFrame.
Depends on: 1242029
Assignee: nobody → rcaliman
Blocks: 1242029
No longer depends on: 1242029
Comment on attachment 8970835 [details]
Bug 1455588 - Allow dragging Shape Editor markers outside of iframe viewport.

https://reviewboard.mozilla.org/r/239616/#review245286

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:303
(Diff revision 1)
> -  info("Moving inset bottom");
> -  onShapeChangeApplied = highlighters.once("shapes-highlighter-changes-applied");
> -  await mouse.down(xCenter, bottom, selector);
> -  await mouse.move(xCenter, bottom + dy, selector);
> -  await mouse.up(xCenter, bottom + dy, selector);
> -  await testActor.reflow();
> -  await onShapeChangeApplied;
> +  // NOTE: Skip testing bottom inset marker until Bug 1456777 is fixed.
> +  // This fails in headless mode becasue the viewport is smaller than the tested
> +  // element and the bottom inset marker is outside the viewport. Moving it clamps it.
> +  // Bug 1456777 - https://bugzilla.mozilla.org/show_bug.cgi?id=1456777
> +  //
> +  // info("Moving inset bottom");
> +  // onShapeChangeApplied = highlighters.once("shapes-highlighter-changes-applied");
> +  // await mouse.down(xCenter, bottom, selector);
> +  // await mouse.move(xCenter, bottom + dy, selector);
> +  // await mouse.up(xCenter, bottom + dy, selector);
> +  // await testActor.reflow();
> +  // await onShapeChangeApplied;

Do you intend to work on bug 1456777 right away? Or is this a nice to have for later?
If the later, then please just delete the code and add a single TODO comment line instead. No use keeping dead code around for long periods of time.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:304
(Diff revision 1)
>    await mouse.up(xCenter, top + dy, selector);
>    await testActor.reflow();
>    await onShapeChangeApplied;
>  
> -  info("Moving inset bottom");
> -  onShapeChangeApplied = highlighters.once("shapes-highlighter-changes-applied");
> +  // NOTE: Skip testing bottom inset marker until Bug 1456777 is fixed.
> +  // This fails in headless mode becasue the viewport is smaller than the tested

s/becasue/because

::: devtools/server/actors/highlighters/shapes.js:458
(Diff revision 1)
> -    const top = pageYOffset + padding;
> -    const bottom = innerHeight + pageYOffset - padding;
> +    // points can be dragged to the extent of the global window, outside of the iframe
> +    // window.
> +    if (this.currentNode.ownerGlobal !== this.win) {
> +      const win =  this.win;
> +      const nodeWin = this.currentNode.ownerGlobal;
> +      [xOffset, yOffset] = getFrameOffsets(win, nodeWin);

I'm not sure why we still use `getFrameOffsets`in our code knowing that `getBoxQuads({relativeto})` is very useful.

I believe in your case, you could do:

`node.getBoxQuads({relativeTo: this.win.document});`

or something like that.
Attachment #8970835 - Flags: review?(pbrosset) → review+
Comment on attachment 8970835 [details]
Bug 1455588 - Allow dragging Shape Editor markers outside of iframe viewport.

https://reviewboard.mozilla.org/r/239616/#review245286

> I'm not sure why we still use `getFrameOffsets`in our code knowing that `getBoxQuads({relativeto})` is very useful.
> 
> I believe in your case, you could do:
> 
> `node.getBoxQuads({relativeTo: this.win.document});`
> 
> or something like that.

`getBoxQuads` is such a valuable utility that I keep forgetting about. Thanks for the reminder! :) 

It works great in this instance.

I tried replacing the other use of `getFrameOffsets()` in this file, but in that case it seems to have issues with scrolled iframes. I will investigate replacing it there at a later date.
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07e88c223397
Allow dragging Shape Editor markers outside of iframe viewport. r=pbro
https://hg.mozilla.org/mozilla-central/rev/07e88c223397
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.