1262686 - Don't use margin-left on PopupAutoCompleteRichResult with RTL locales

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[magicp]

Attachment: rtl-PopupAutoCompleteRichResult-margin-inline-start.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160406030221

Steps to reproduce:

1. Start latest Nightly with RTL locales (e.g. Arabic)
2. Enter any keywords in urlbar


Actual results:

The PopupAutoCompleteRichResult does not fit into Firefox window size by wrong direction of margin.


Expected results:

The PopupAutoCompleteRichResult should be fit into Firefox window size.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c131636dae420773342d1439b6086b6c62b222f4&tochange=061165ac1ff9e076ec51ea268878efa751173511

Comment 2

[Drew Willcoxon :adw]

Attachment: MozReview Request: Bug 1262686 - Properly align awesomebar popup with window on RTL locales. r?mak

Tested with Force RTL.

Review commit: https://reviewboard.mozilla.org/r/45059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45059/

Comment 3

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae329ac38659

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8739137 [details]
MozReview Request: Bug 1262686 - Properly align awesomebar popup with window on RTL locales. r?mak

https://reviewboard.mozilla.org/r/45059/#review41587

::: browser/base/content/urlbarBindings.xml:1365
(Diff revision 1)
>            var popupDirection = aElement.ownerDocument.defaultView.getComputedStyle(aElement).direction;
>            this.style.direction = popupDirection;
>  
> -          // Move left margin to the window border.
> -          let elementRect = aElement.getBoundingClientRect();
> -          this.style.marginLeft = "-" + (elementRect.left - rect.left) + "px";
> +          // Make the popup's starting margin negaxtive so that the start of the
> +          // popup aligns with the window border.
> +          if (popupDirection == "rtl") {

Where is elementRect defined? You appear to have removed its definition....

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8739137 [details]
MozReview Request: Bug 1262686 - Properly align awesomebar popup with window on RTL locales. r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45059/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cab24273ca0

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8739137 [details]
MozReview Request: Bug 1262686 - Properly align awesomebar popup with window on RTL locales. r?mak

https://reviewboard.mozilla.org/r/45059/#review41591

::: browser/base/content/urlbarBindings.xml:1368
(Diff revision 2)
> -          // Move left margin to the window border.
> +          // Make the popup's starting margin negaxtive so that the start of the
> +          // popup aligns with the window border.
>            let elementRect = aElement.getBoundingClientRect();
> -          this.style.marginLeft = "-" + (elementRect.left - rect.left) + "px";
> +          if (popupDirection == "rtl") {
> +            let offset = elementRect.right - rect.right
> +            this.style.marginRight = offset + "px";

Can we use style.MozMarginStart and have it do black magic for us?

Comment 8

[Drew Willcoxon :adw]

That was the first thing I tried, but it didn't have any effect in LTR mode (didn't try RTL).  I looked around in nsMenuPopupFrame.cpp but couldn't see why.  I don't know if -moz-margin-start doesn't get parsed into the nsStyleMargin struct or what.

But even if it did work, we still need to calculate the offset differently in the different directions.  RTL uses rect.right's and LTR uses rect.left's, and as far as I know there's no rect.start.  So there's not much benefit to using a single CSS selector vs. two anyway.

Comment 9

[magicp]

// Move left margin to the window border.
let elementRect = aElement.getBoundingClientRect();
if (popupDirection == "rtl") {
  this.style.marginRight = "-" + (rect.right - elementRect.right) + "px";
} else {
  this.style.marginLeft = "-" + (elementRect.left - rect.left) + "px";
}

Comment 10

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/0e8c81f5e47c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e8c81f5e47c

Comment 12

[Drew Willcoxon :adw]

A couple of more RTL bugs: bug 1263674, bug 1263698.

Comment 13

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed across platforms using Latest 48.0a2 DevEdition, buildID 20160503004116.
Tested using the Arabic locale and by forcing en-US to be RTL (created intl.uidirection.en string set to rtl)