Closed Bug 1119057 Opened 10 years ago Closed 10 years ago

[RTL][Browser] Scroll bar is on the wrong side.

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::System::Browser Chrome
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(feature-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 verified, b2g-master verified)

Status:
VERIFIED FIXED
Milestone:
2.2 S6 (20feb)
Project Flags:
feature-b2g 2.2+
Tracking Flags:
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: ychung, Assigned: kaze)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

ScrollBarOnRight.png
223.14 KB, image/png
Details
scrollbar position should follow the document `dir`
1.98 KB, patch
timdream
: review+
bajaj
: approval-gaia-v2.2+
Details | Diff | Splinter Review
Attached image ScrollBarOnRight.png —
Description: The scroll bar is located on the right when a webpage is displayed. Repro Steps: 1) Update a Flame device to BuildID: 20150107010216. 2) Open Browser app, and navigate to a webpage that can be scrolled. 3) Scroll down the page. Actual: Scroll bar is located on the right. Expected: Scroll bar is located on the left. Environmental Variables: Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash) BuildID: 20150107010216 Gaia: 69ac77cfa938fae2763ac426a80ca6e5feb6ad25 Gecko: 33781a3a5201 Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76 Version: 37.0a1 (2.2 Master) Firmware: V188-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Repro frequency: 100% See attached: screenshot
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(ktucker)
Whiteboard: [systemsfe]
Given most of the apps have scroll bars as exoected on the left, we should try to be consistent and fix this.
blocking-b2g: --- → 2.2+
Priority: -- → P1
I didn't think we were blocking on RTL bugs in 2.2? This may be a platform issue.
Flags: needinfo?(bbajaj)
(In reply to Ben Francis [:benfrancis] from comment #2) > I didn't think we were blocking on RTL bugs in 2.2? This may be a platform > issue. So that explains why all my efforts put into this didn't work!
(In reply to Ben Francis [:benfrancis] from comment #2) > I didn't think we were blocking on RTL bugs in 2.2? This may be a platform > issue. we are indeed helping uplifting as much as we can in 2.2 with Ahmed's help. So you are saying this is not a browser issue and someone from layout (or ? ) needs to look at it ?
Flags: needinfo?(bbajaj) → needinfo?(bfrancis)
I can't figure out where this is set. I thought it was just a system-wide pref which should get switched when you change language, but that doesn't explain why it seems like some apps have the scrollbar on the left but browser windows and other apps (like homescreen) have it on the right. Ahmed, do you know if individual apps are setting this in CSS? I assumed it was just a system-wide setting.
Flags: needinfo?(bfrancis) → needinfo?(nefzaoui)
RTL update: marking required bugs as feature-b2g:2.2+ (and removing blocking flags)
blocking-b2g: 2.2+ → ---
feature-b2g: --- → 2.2+
Hey Ben, taking this bug. :-) (In reply to Ben Francis [:benfrancis] from comment #5) > I can't figure out where this is set. Me neither, and I’ve been looking for that for a couple hours now… > Ahmed, do you know if individual apps are setting this in CSS? I assumed it > was just a system-wide setting. AFAIK, for all Gaia apps /except/ Browser and Homescreen, switching the document direction magically mirrors the scrollbar position. The document.documentElement.dir is changed by l10n.js itself, but it works fine as well if you just set the `dir` attribute on the main <html> element in WebIDE. Of course, there’s no way to do so on a Browser tab; and setting the mozbrowser iframe `dir` attribute doesn’t help either. Any hint would be very appreciated. :-) Who can we NI about this?
Assignee: nobody → fabien
Flags: needinfo?(nefzaoui) → needinfo?(bfrancis)
Status: NEW → ASSIGNED
David, can you help us out here? How do we support scroll bars on left for web content that is in English when the phone itself is in RTL mode? Is this possible or even desirable to have scroll bars on the left here?
Flags: needinfo?(dbaron)
Just to be clear: we want to have scrollbars on the left in Browser tabs when the phone is set to an RTL language such as Arabic — like it already works for other Gaia apps (Settings, Music, Contacts…). This is not related to the language of the web content itself.
(In reply to Michael Henretty [:mhenretty] from comment #8) > David, can you help us out here? How do we support scroll bars on left for > web content that is in English when the phone itself is in RTL mode? Is this > possible or even desirable to have scroll bars on the left here? For Gecko we have a pref with 4 options: https://hg.mozilla.org/mozilla-central/file/dece5291d037/modules/libpref/init/all.js#l2212 though this pref only applies to the root scrollbar in the document and not to the ones inside of it. Of course, the default for that pref says that we follow the UI language, not the content language. The code that applies the pref is in ScrollFrameHelper::IsScrollbarOnRight in layout/generic/nsGfxScrollFrame.cpp -- fiddling with that should show whether it's relevant to the way we do scrollbars on B2G (I hope so!) and/or what's not doing what you expect.
Flags: needinfo?(dbaron)
(I could believe that we might be setting up mIsRoot incorrectly or something like that.)
Already commented about this in Bug 1126211: Browser is not the only area affected. There is also Rocketbar search, Homescreen, Marketplace, Smart Collections. Scroll bar should be mirrored for RTL on these areas and is not, as per UX specs p.16 Should I open up separate bugs for each and every case?
Rocketbar search, homescreen and smart collections are all loaded in special window types in the window manager, which may be the common element. I'm not sure about Marketplace, but maybe it's deriving its direction from the direction from the Marketplace app content itself? Are other apps setting their direction manually in CSS?
Flags: needinfo?(bfrancis)
Looking at layout/generic/nsGfxScrollFrame.cpp right now, thanks for the pointer David. (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #10) > For Gecko we have a pref with 4 options: > https://hg.mozilla.org/mozilla-central/file/dece5291d037/modules/libpref/ > init/all.js#l2212 > though this pref only applies to the root scrollbar in the document and not > to the ones inside of it. Actually, it looks like the opposite happens: • most Gaia apps have a “body{overflow:hidden}” rule (e.g. Settings), the scrollable content is in an inner <div> or <section> node, and they display the scrollbar on the expected end side • for other apps that rely on <body> to scroll their contents (e.g. Homescreen), the scrollbar stays on the right for RTL documents. (In reply to Ben Francis [:benfrancis] from comment #13) > Rocketbar search, homescreen and smart collections are all loaded in special > window types in the window manager, which may be the common element. Please explain the “special window types” you’re thinking of. I may have missed that point.
Flags: needinfo?(bfrancis)
> Please explain the “special window types” you’re thinking of. I may have missed that point. Just that they are not normal AppWindows. They are opened in SearchWindow or a HomescreenWindow.
Flags: needinfo?(bfrancis)
This bug only affects root scrollbars. Non-root scrollbars work as expected in RTL mode. It turns out that the current logic is: • when layout.scrollbar.side == 0, the root scrollbar position is decided by `bidi.direction`; • when layout.scrollbar.side == 1, the document `dir` is used instead; • and the position of non-root scrollbars always follows the document `dir`. As `bidi.direction` is not set along with `general.useragent.locale` and `intl.accept_languages` when the locale is changed, root scrollbars are always displayed on the right edge with the default setting (layout.scrollbar.side = 0).
• pro: the root scrollbar position is properly set to the expected edge without having to restart apps (works with all Gaia apps); • con: for Browser tabs that are not in RTL mode, the root scrollbar will be on the left edge. I feel like having an LTR scrollbar with LTR web pages is OK, but that would be a UX decision. Ahmed, what do you think?
Flags: needinfo?(nefzaoui)
Attachment #8557909 - Flags: review?(21)
(In reply to Fabien Cazenave [:kaze] from comment #18) > Created attachment 8557909 [details] [diff] [review] > scrollbar position should follow the document `dir` > > • pro: the root scrollbar position is properly set to the expected edge > without having to restart apps (works with all Gaia apps); > • con: for Browser tabs that are not in RTL mode, the root scrollbar will be > on the left edge. > > I feel like having an LTR scrollbar with LTR web pages is OK, but that would > be a UX decision. Ahmed, what do you think? From my perspective, I wouldn't say it's a top priority bug since the "damage" of not fixing it is at its minimal in terms of UI territories (I myself didn't notice this myself until someone pointed it out and filed the bug) But if we have a fix then let's for it :)
Flags: needinfo?(nefzaoui)
FTR, here’s a more “natural” fix for this bug: • pro: root scrollbars will always be on the left edge in RTL mode, even for LTR documents in the Browser; • con: when switching from an LTR locale to a RTL one (or vice-versa), root scrollbars are hidden until the application is restarted. ScrollFrameHelper::IsScrollbarOnRight relies on a cached value of `bidi.direction` to return the root scrollbar position, but getting a fresh value of `bidi.direction` is not enough to set the scrollbar position properly. Robert, could you help us here? It would be really nice to have a patch to refresh the scrollbar position when `bidi.direction` is updated.
Flags: needinfo?(roc)
(sorry: re-submitting a patch because of a bad typo in the commentary. Hope this clarifies a bit.) • pro: the root scrollbar position is properly set to the expected edge without having to restart apps (works with all Gaia apps); • con: the scrollbar position in Browser tabs depends on the *document* direction, not the chosen locale. In other words, it will be on the right edge for LTR documents and on the left edge for RTL documents. I wouldn’t bother at all (in fact, I’d even prefer that — the scrollbar is only visible while panning anyway) but again, that’s a UX decision.
Attachment #8557909 - Attachment is obsolete: true
Attachment #8557909 - Flags: review?(21)
Attachment #8557944 - Flags: review?(21)
(In reply to Fabien Cazenave [:kaze] from comment #20) > Robert, could you help us here? It would be really nice to have a patch to > refresh the scrollbar position when `bidi.direction` is updated. Changing bidi.direction between 1 and 2 works for me on desktop (with layout.scrollbar.side == 0). ScrollFrameHelper::IsScrollbarOnRight checks layout.scrollbar.side (a.k.a. kPresContext_ScrollbarSide) and if necessary, bidi.direction. Maybe your problem is that if layout.scrollbar.side changes, we don't set mPrefChangePendingNeedsReflow like we do when bidi.direction changes? We should, in nsPresContext::PreferenceChanged. But I kinda doubt that's your problem. I'm out of ideas. I'd need a frame tree dump to say more.
Flags: needinfo?(roc)
Comment on attachment 8557944 [details] [diff] [review] scrollbar position should follow the document `dir` Redirecting the review, as Vivien is on PTO.
Attachment #8557944 - Flags: review?(21) → review?(timdream)
Comment on attachment 8557944 [details] [diff] [review] scrollbar position should follow the document `dir` Please file follow-up bugs to ensure <iframe mozbrowser> follows the same behavior as <xul:tabbrowser>. We should not be diverge the behavior of FxOS and Fx.
Attachment #8557944 - Flags: review?(timdream) → review+
Target Milestone: --- → 2.2 S6 (20feb)
Fabien, can we land this?
Flags: needinfo?(fabien)
Yes we can (™). I don’t have the rights to do so — adding the `checkin-needed` keyword.
Flags: needinfo?(fabien)
Keywords: checkin-needed
Please provide a Try link first. Also, to avoid any confusion, please mark any obsolete patches as such.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #28) > Please provide a Try link first. Also, to avoid any confusion, please mark > any obsolete patches as such.
Flags: needinfo?(fabien)
Attachment #8557933 - Attachment is obsolete: true
Flags: needinfo?(fabien)
Summary: [RTL][Browser] Scroll bar is on the right side. → [RTL][Browser] Scroll bar is on the wrong side.
https://hg.mozilla.org/mozilla-central/rev/12391199c45f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Comment on attachment 8557944 [details] [diff] [review] scrollbar position should follow the document `dir` [Approval Request Comment] [User impact] if declined: major — scrollbars on the wrong side in RTL mode [Testing completed]: manual [Risk to taking this patch] (and alternatives if risky): low — just a pref update [String changes made]: none
Attachment #8557944 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8557944 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
When viewing english websites while the phone is set to Arabic language (RTL) the scrollbar is still on the right (example: Kotaku.com). Sites that translate to Arabic show the scrollbar on the left as expected (example: Google.com). This occurs on both the 3.0 and 2.2 Nightly Flame builds. Environmental Variables: Device: Flame 3.0 KK (319MB) (Full Flash) BuildID: 20150220010206 Gaia: e4f7c67378e33e83f88d38ddb4a6c2cabf1423c3 Gecko: 1b4c5daa7b7a Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Environmental Variables: Device: Flame 2.2 KK (319MB) (Full Flash) BuildID: 20150220002501 Gaia: ce79d35b92261e7cbfeaefebf87859ebeb0979b4 Gecko: b864abe1c6b3 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0a2 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage+][rtl-impact] → [QAnalyst-Triage+][rtl-impact][failed-verification]
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
Test case has been added in moztrap: https://moztrap.mozilla.org/manage/case/15483/
Flags: in-moztrap+
Per comment 37, I think this is due to which language the website used, and in my opinion, this bug has been verified successfully. Hi Delphine, Could you help to confirm this? Thanks!
Flags: needinfo?(lebedel.delphine)
Correct! thanks all
Flags: needinfo?(lebedel.delphine)
this marking as verified fixed :)
Status: RESOLVED → VERIFIED
Thanks Delphine.
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
QA Whiteboard: [QAnalyst-Triage+][rtl-impact][failed-verification] → [QAnalyst-Triage+][rtl-impact]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: