[RTL] Feed popup is hardcoded to LTR

RESOLVED FIXED in Firefox 3.6a1

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: tomer, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {rtl, verified1.9.1})

Trunk
Firefox 3.6a1
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.1 .4-fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

11 years ago
When using RTL version of Firefox, the feed selection is LTR instead of RTL. 

Steps to reproduce:
1. Launch Hebrew [Arabic? Farsi?] Firefox.
2. Click on the RSS icon on site that contain more than one feed per page.

Example sites:
http://linmagazine.co.il
http://www.ynet.co.il/articles/0,7340,L-3248668,00.html
Assignee

Updated

11 years ago

Comment 1

11 years ago
looks like my bugzilla privileges are evaporating. this is the same on windows with beta4, but i can't change the OS here myself.
Reporter

Comment 2

11 years ago
Thanks for the quick review, Tsahi.
Component: he-IL / Hebrew → Theme
OS: Linux → All
Product: Mozilla Localizations → Firefox
QA Contact: hebrew.he → theme
Version: unspecified → Trunk
Is this a dupe of bug 353087?
Reporter

Comment 4

11 years ago
(In reply to comment #3)
> Is this a dupe of bug 353087?
> 

Probably not. That bug talk about the feed preview page, while this one is about the feed selector popup for sites with multiple feed choices. But I like the other bug.

Comment 5

11 years ago
tsahi_75@yahoo.com, I've adjusted your permissions so you should be able to edit bugs. 
Assignee

Updated

11 years ago
Blocks: fx35-l10n-fa
Assignee

Updated

11 years ago
No longer blocks: Persian-Fx3.5
Assignee

Comment 6

11 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Assignee

Comment 7

11 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
Part of this bug (regarding the direction of the menu) is like bug 473440.  Another part is that the position of the menu is incorrect (like bug 427739).  The automated test tests the second part.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #358710 - Flags: review?(gavin.sharp)
Assignee

Comment 8

11 years ago
We need a Litmus test for the first part of this bug (the directionality of the menu itself).
Flags: in-testsuite?
Flags: in-litmus?
Assignee

Updated

11 years ago
Attachment #358710 - Flags: review?(gavin.sharp) → review?(mconnor)
Assignee

Updated

11 years ago
Whiteboard: [has patch][needs review mconnor]
Wouldn't this be better fixed by changing the meaning of "after_start" and "after_end" in the popup code when the chrome direction is RTL? It doesn't seem ideal to make these changes everywhere we open popups (bug 427739, this bug, etc.).
Assignee

Comment 10

11 years ago
In bug 477754 comment 8, Neil mentioned that popup directions are handled correctly if the popup's frame is RTL.  I'm not sure why that doesn't work correctly in this case.  Neil, can you please clarify?
The location bar is ltr.
Assignee

Comment 12

11 years ago
(In reply to comment #11)
> The location bar is ltr.

Yes, but with this patch I set the direction of the menupopup element which is the child of feed-button to rtl explicitly.  I also tried setting the direction of feed-button to rtl as well, but in both cases the direction of the menu was incorrect.

Comment 13

11 years ago
Why is the location bar left-to-right, shouldn't it actually be the input inside it?
Assignee

Comment 14

11 years ago
There is no general rule here, but it was decided that the site button should be kept next to the URL, which would be at the left side.  Given that, the star and feed icons would have to appear at the right side, and the only thing which was "mirrored" for RTL locales in the location bar was the drop-down arrow.
Assignee

Comment 15

10 years ago
Mconnor: ping?
Assignee

Updated

10 years ago
No longer blocks: fx35-l10n-fa
Assignee

Updated

10 years ago
Blocks: Persian
Attachment #358710 - Flags: review?(mconnor) → review?(dao)
Comment on attachment 358710 [details] [diff] [review]
Patch (v1)

random-review-rebalancing wheel says... Dao!
Comment on attachment 358710 [details] [diff] [review]
Patch (v1)

>+#feed-button > menupopup[chromedir="rtl"] {
>+  direction: rtl;
> }

how about:

#feed-menu[chromedir="rtl"] > menuitem {
  direction: rtl;
}
Comment on attachment 358710 [details] [diff] [review]
Patch (v1)

See previous comment... seems like FeedHandler.adjustPosition can be avoided.
Attachment #358710 - Flags: review?(dao) → review-
Assignee

Comment 19

10 years ago
Posted patch Patch (v2)Splinter Review
(In reply to comment #18)
> (From update of attachment 358710 [details] [diff] [review])
> See previous comment... seems like FeedHandler.adjustPosition can be avoided.

You're absolutely right.  Here is the new patch.
Attachment #358710 - Attachment is obsolete: true
Attachment #390707 - Flags: review?(aseemsethi)
Assignee

Updated

10 years ago
Attachment #390707 - Flags: review?(aseemsethi) → review?(dao)
Assignee

Updated

10 years ago
status1.9.1: --- → ?
Flags: in-testsuite?
Whiteboard: [has patch][needs review mconnor] → [has patch][needs r=dao]
Attachment #390707 - Flags: review?(dao) → review+
Component: Theme → General
QA Contact: theme → general
Assignee

Comment 20

10 years ago
http://hg.mozilla.org/mozilla-central/rev/96f872736e8a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs r=dao]
Target Milestone: --- → Firefox 3.6a1
Assignee

Updated

10 years ago
Attachment #390707 - Flags: approval1.9.1.2?
Comment on attachment 390707 [details] [diff] [review]
Patch (v2)

This needs to bake before we take it on 1.9.1.
Attachment #390707 - Flags: approval1.9.1.2? → approval1.9.1.3?
Comment on attachment 390707 [details] [diff] [review]
Patch (v2)

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #390707 - Flags: approval1.9.1.3? → approval1.9.1.4+
Verified for 1.9.1.4 with Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9.1.4pre) Gecko/20090921 Shiretoko/3.5.4pre.
Keywords: verified1.9.1
Assignee

Updated

6 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.