Page MenuHomePhabricator

On RTL wiki, context menu on left-floated block image expands the document width, causing an ugly horizontal scrollbar
Closed, ResolvedPublic8 Estimated Story Points

Description

Clicking on file opens popup widget, which appear outside the content (left to it) and creates horizontal scrollbar:

MediaWidgetHorizontalScrollbar.png (702×497 px, 79 KB)

(On hewiki production)

Event Timeline

Jdforrester-WMF renamed this task from Popup widgets creates ugly horizontal scrollbar to [Regression?] On RTL wiki, context menu on left-floated block image expands the document width, causing an ugly horizontal scrollbar.Mar 2 2017, 6:53 PM
matmarex subscribed.

Seems to be an OOjs UI regression from 799f500934eb64d0623d06f3e7d452d8058db647. PopupWidget#updateDimensions detection of whether the popup will be beyond edge of screen is wrong for RTL.

I think this might work, but I didn't test it in all of the combinations of LTR/RTL and left/right edge.

I'm pretty sure about the first change (originOffset is meant to be position of the anchor, but in RTL it was position of left edge). I'm less sure about the second change, the negation is wrong in some circumstances, but I still needed to test that.

I'm off for today, I'll get back to this tomorrow if no one else does it first.

diff --git a/src/widgets/PopupWidget.js b/src/widgets/PopupWidget.js
index d11961d..1e2362e 100644
--- a/src/widgets/PopupWidget.js
+++ b/src/widgets/PopupWidget.js
@@ -355,6 +355,9 @@ OO.ui.PopupWidget.prototype.updateDimensions = function ( transition ) {

        // Figure out if this will cause the popup to go beyond the edge of the container
        originOffset = this.$element.offset().left;
+       if ( direction === 'rtl' ) {
+               originOffset += this.$element.width();
+       }
        containerLeft = this.$container.offset().left;
        containerWidth = this.$container.innerWidth();
        containerRight = containerLeft + containerWidth;
@@ -365,9 +368,9 @@ OO.ui.PopupWidget.prototype.updateDimensions = function ( transition ) {

        // Adjust offset to make the popup not go beyond the edge, if needed
        if ( overlapRight < 0 ) {
-               popupOffset += dirFactor * overlapRight;
+               popupOffset += overlapRight;
        } else if ( overlapLeft < 0 ) {
-               popupOffset -= dirFactor * overlapLeft;
+               popupOffset -= overlapLeft;
        }

        // Adjust offset to avoid anchor being rendered too close to the edge
Jdforrester-WMF moved this task from To Triage to TR0: Interrupt on the VisualEditor board.

With my PopupWidget rewrite patch the alignment works but the page is still stretched. With my (unsubmitted) local changes to that patch, the page stretching is gone too.

Change 341038 had a related patch set uploaded (by catrope):
[VisualEditor/VisualEditor] DesktopContext: Unhide context before positioning popup

https://gerrit.wikimedia.org/r/341038

With my PopupWidget rewrite patch the alignment works but the page is still stretched. With my (unsubmitted) local changes to that patch, the page stretching is gone too.

My changes to that patch are now in Gerrit and ready for review, and together with the DesktopContext patch (and https://gerrit.wikimedia.org/r/#/c/340916/ ), it fixes the bug:

rtl-context-fixed.png (427×439 px, 77 KB)

Jdforrester-WMF renamed this task from [Regression?] On RTL wiki, context menu on left-floated block image expands the document width, causing an ugly horizontal scrollbar to On RTL wiki, context menu on left-floated block image expands the document width, causing an ugly horizontal scrollbar.Mar 3 2017, 8:26 PM
Jdforrester-WMF set the point value for this task to 8.

Change 341038 merged by jenkins-bot:
[VisualEditor/VisualEditor] DesktopContext: Unhide context before positioning popup

https://gerrit.wikimedia.org/r/341038

Change 341820 had a related patch set uploaded (by Jforrester):
[mediawiki/extensions/VisualEditor] Update VE core submodule to master (de2dd95c6)

https://gerrit.wikimedia.org/r/341820

Change 341820 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor] Update VE core submodule to master (de2dd95c6)

https://gerrit.wikimedia.org/r/341820