Split screenshot iframe into preselection and selection iframes #2453
Conversation
4899e7d
to
d4bc0c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! But some comments on updateElementSize and removal of anything related to scroll tracking.
addon/webextension/selector/ui.js
Outdated
this.element.style.display = "none"; | ||
} | ||
let height = Math.max( | ||
document.documentElement.clientHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateElementSize
can be simpler for this iframe. It should always be exactly window.innerHeight
(and width), and doesn't have to track document changes. force
also doesn't matter for this iframe.
addon/webextension/selector/ui.js
Outdated
}, | ||
|
||
hide: function () { | ||
window.removeEventListener("scroll", this.onScroll, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything related to scroll tracking can be removed. Neither iframe should require it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hoverbox remains visible during the scroll without this. I think it looks bad, but if that's not an issue I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't catch that part. That makes sense then.
addon/webextension/selector/ui.js
Outdated
currentIframe: iframePreSelection, | ||
display: function (installHandlerOnDocument, standardOverlayCallbacks) { | ||
return iframeSelection.display(installHandlerOnDocument) | ||
.then(() => iframePreSelection.display(installHandlerOnDocument, standardOverlayCallbacks)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;
@@ -636,8 +636,7 @@ window.uicontrol = (function () { | |||
stateHandlers.resizing.startResize(event, "move"); | |||
} else if (! ui.Box.isControl(target)) { | |||
mousedownPos = new Pos(event.pageX, event.pageY); | |||
mouseupNoAutoselect = true; | |||
setState("draggingReady"); | |||
setState("crosshairs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we should rename crosshairs as that mode hasn't displayed crosshairs for a long time. But not really related to this PR.
closes #2162