RTL small UI issues after the landing of bug 476423

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 470998 [details] [diff] [review]
Patch

There is still some UI problems when launching Fennec in RTL mode. The patch fix many little CSS/UI of them.
Attachment #470998 - Flags: review?(mark.finkle)
Comment on attachment 470998 [details] [diff] [review]
Patch


>-    let [,,,controlsW] = Browser.computeSidebarVisibility();
>-    this.box.left = window.innerWidth - (this.box.getBoundingClientRect().width + controlsW + margin);
>-    this.box.top  = BrowserUI.starButton.getBoundingClientRect().top + margin;
>+    let [,,leftW,rightW] = Browser.computeSidebarVisibility();
>+    let starRect = BrowserUI.starButton.getBoundingClientRect();
>+    let spacer = starRect.left < Elements.tabs.getBoundingClientRect().left ? (leftW + margin)
>+                                                                            : -(rightW + margin + this.box.getBoundingClientRect().width);
>+    this.box.left = starRect.left + spacer;
>+    this.box.top  = starRect.top + margin;

I assume this is because we open the popup "near" the startButton?

>diff -r bb790bfcba55 chrome/content/browser.js

>-    let leftbarCBR = document.getElementById('tabs-container').getBoundingClientRect();
>-    let ritebarCBR = document.getElementById('browser-controls').getBoundingClientRect();
>+    let leftbarCBR = Elements.tabs.getBoundingClientRect();
>+    let ritebarCBR = Elements.controls.getBoundingClientRect();

rite* -> right* PLEASE! Don't keep the mistake alive!

>diff -r bb790bfcba55 themes/core/browser.css

> box[type="documenttab"] {
>   /* display:block allow us to change the line-height, it won't work otherwise */
>   display: block;
>   width: 128px;
>+  margin-left: 8px;
>   line-height: 0;
> }

>+box[type="documenttab"]:-moz-locale-dir(rtl) {
>+  margin-left: 0px;
>+  margin-right: 8px;
>+}

>+.documenttab-close-container:-moz-locale-dir(rtl) {
>+  margin-left: 65px;
>+}

Can't we use -moz-margin-start or -moz-margin-end here? It seems wrong to be introducing "left" and "right" back into the code. What am I missing?
Created attachment 471066 [details] [diff] [review]
Patch v0.2

Addressed comments.


(In reply to comment #1)
> Comment on attachment 470998 [details] [diff] [review]
> Patch
> 
> 
> >-    let [,,,controlsW] = Browser.computeSidebarVisibility();
> >-    this.box.left = window.innerWidth - (this.box.getBoundingClientRect().width + controlsW + margin);
> >-    this.box.top  = BrowserUI.starButton.getBoundingClientRect().top + margin;
> >+    let [,,leftW,rightW] = Browser.computeSidebarVisibility();
> >+    let starRect = BrowserUI.starButton.getBoundingClientRect();
> >+    let spacer = starRect.left < Elements.tabs.getBoundingClientRect().left ? (leftW + margin)
> >+                                                                            : -(rightW + margin + this.box.getBoundingClientRect().width);
> >+    this.box.left = starRect.left + spacer;
> >+    this.box.top  = starRect.top + margin;
> 
> I assume this is because we open the popup "near" the startButton?

Yep, the (new) patch also fixes some others popups for RTL (NewTabPopup, AlertsContainer)
Attachment #470998 - Attachment is obsolete: true
Attachment #471066 - Flags: review?(mark.finkle)
Attachment #470998 - Flags: review?(mark.finkle)
Comment on attachment 471066 [details] [diff] [review]
Patch v0.2

Can we add a getter for the bookmark popup "box" so the calculation for RTL is only don once? Like you did for the other popups?

r+, but if you can tweak the getter, that would be nice. if not, or it's more work, we can file a followup bug.
Attachment #471066 - Flags: review?(mark.finkle) → review+
(In reply to comment #3)
> Comment on attachment 471066 [details] [diff] [review]
> Patch v0.2
> 
> Can we add a getter for the bookmark popup "box" so the calculation for RTL is
> only don once? Like you did for the other popups?
> 
> r+, but if you can tweak the getter, that would be nice. if not, or it's more
> work, we can file a followup bug.

I've tweak the code to go into the getter

http://hg.mozilla.org/mobile-browser/rev/13b4414619c2
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
bugspam
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.