635522 - [rtl] tab close buttons in the wrong spot

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Kevin Brosnan [:kbrosnan]]

Attachment: close buttons in the wrong spot

Using the Arabic language pack the tab close buttons are completely over the tabs. The edge of the x should be over the right edge of the tab.

Firefox Mobile 4 beta 5 candidate 3
Android 2.2 HTC G2 (Desire Z)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

low priority blocker

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch (m-c)

The patch add start/end attributes support to <stack/> element. In some cases this make life simpler when dealing with a UI that support RTL.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch (Front-End)

Comment 4

[Neil Deakin]

Comment on attachment 514775 [details] [diff] [review]
Patch (m-c)

Good, but please add some tests for left/right/start/end in rtl mode as well.

The idea here is that left and right attributes override the start and end attributes. It would good to have a followup bug to improve this code a bit, such that:
 - it doesn't need to check start/end if left and/or right were specified.
 - it doesn't duplicate so much code

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch (m-c) for checkin

Same patch but with some rtl tests.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 514776 [details] [diff] [review]
Patch (Front-End)


>       if (Browser.floatedWhileDragging) {

Add a comment here to say: We need to adjust for RTL layout

>-        let [leftVis,,leftW,] = Browser.computeSidebarVisibility();
>-        x = Math.round(Math.max(0, leftW * leftVis));
>+        let [leftVis, rightVis, leftW, rightW] = Browser.computeSidebarVisibility();
>+        let [leftSidebar, rightSidebar] = [Elements.tabs.getBoundingClientRect(), Elements.controls.getBoundingClientRect()];
>+        if (leftSidebar.left > rightSidebar.left)
>+          x = Math.round(Math.max(0, rightW * rightVis));
>+        else
>+          x = Math.round(Math.max(0, leftW * leftVis));

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 515116 [details] [diff] [review]
Patch (m-c) for checkin

Make sure you file the followup bug suggested by Neil in comment 4

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

pushed the m-c part: http://hg.mozilla.org/mozilla-central/rev/a1d3010eff92

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

pushed the front-end fix: http://hg.mozilla.org/mobile-browser/rev/fb597ad4bfa7

Filed bug 637514 as a followup for the code clean-up:

Comment 10

[Ioana Chiorean]

Build id: Mozilla/5.0 (Android;Linux armv7l;rv:2.0b13pre)Gecko/20110301
Firefox/4.0b13pre Fennec /4.0b6pre 

The close x icon is still in the wrong position. I will reopen the bug.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch followup (front-end)

Gasp, I've noticed some left-over in tabs.xml before checking and I've remove too much code.
The patch change "left" by "start" into tabs.xml (and add some css rules for the navigation buttons in rtl)

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Pushed the followup: http://hg.mozilla.org/mobile-browser/rev/825f90dd0e44

Comment 13

[Ioana Chiorean]

verified fixed on build id: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre)
Gecko/20110302
Firefox/4.0b13pre Fennec /4.0b6pre