Fix popup for highlights without popup (follow-up of #12505) #12585
Conversation
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4e0cb532ab3a25f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/d1c35a04be45b0b/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/4e0cb532ab3a25f/output.txt Total script time: 24.87 mins
Image differences available at: http://54.67.70.0:8877/4e0cb532ab3a25f/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/d1c35a04be45b0b/output.txt Total script time: 27.92 mins
Image differences available at: http://3.101.106.178:8877/d1c35a04be45b0b/reftest-analyzer.html#web=eq.log |
| @@ -262,7 +262,9 @@ class AnnotationElement { | |||
| quadPoint[1].y, | |||
| ]; | |||
| this.data.rect = rect; | |||
| quadrilaterals.push(this._createContainer(ignoreBorder)); | |||
| const quad = this._createContainer(ignoreBorder); | |||
| quad.className = "highlightAnnotation"; | |||
There was a problem hiding this comment.
The _createQuadrilaterals method is generic since multiple annotation types can use quadrilaterals, so this class name should not be set here. Fortunately, it can simply be fixed locally in the HighlightAnnotationElement class below.
Hence I'd suggest to revert all changes in this method and instead simply change the render method in the HighlightAnntationElement class to the following (untested):
render() {
if (!this.data.hasPopup) {
this._createPopup(null, this.data);
}
if (this.quadrilaterals) {
this.quadrilaterals.forEach(quadrilateral => {
quadrilateral.className = "highlightAnnotation";
});
return this.quadrilaterals;
}
this.container.className = "highlightAnnotation";
return this.container;
}There was a problem hiding this comment.
@calixteman Hm, it looks like this comment was forgotten to be addressed. Would you mind following this up in a new PR to move this specific class name setting out of the generic function?
There was a problem hiding this comment.
Sorry misread the comment, thought it had been addressed. Filed #12585
|
Given that this fixes a regression, we probably want to make sure this lands before the next "Merge Date" (which according to https://wiki.mozilla.org/Release_Management/Calendar is in less than one week); @calixteman Do you have time to address the outstanding comments here? |
* remove 1st param of _createPopup (almost useless for a method) * prepend popup div to avoid to have them on top of some highlights (and so "disable" partially mouse events) * add a ref test for issue mozilla#12504
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/264635770c21fb2/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/c12239905607351/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/264635770c21fb2/output.txt Total script time: 24.86 mins
Image differences available at: http://54.67.70.0:8877/264635770c21fb2/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/c12239905607351/output.txt Total script time: 27.69 mins
Image differences available at: http://3.101.106.178:8877/c12239905607351/reftest-analyzer.html#web=eq.log |
|
/botio makeref |
From: Bot.io (Linux m4)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e101f4cf9759cb0/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_makeref from @brendandahl received. Current queue size: 0 Live output at: http://3.101.106.178:8877/2c001703aad5ef3/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e101f4cf9759cb0/output.txt Total script time: 24.00 mins
|
From: Bot.io (Windows)SuccessFull output at http://3.101.106.178:8877/2c001703aad5ef3/output.txt Total script time: 29.37 mins
|
…in _createQuadrilaterals
Follow-up for #12585: set elements class in render instead of in _createQuadrilaterals
Fixes #12576.