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

VERIFIED FIXED in Firefox 38, Firefox OS v2.2

Status

P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: ychung, Assigned: kaze)

Tracking

unspecified
2.2 S6 (20feb)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8545555 [details]
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
(Reporter)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?][rtl-impact]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][rtl-impact] → [QAnalyst-Triage+][rtl-impact]
Flags: needinfo?(ktucker)
(Reporter)

Updated

4 years ago
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+
(Assignee)

Comment 7

4 years ago
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)
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 9

4 years ago
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)
Duplicate of this bug: 1126211
(Assignee)

Comment 15

4 years ago
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)
(Assignee)

Comment 17

4 years ago
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).
(Assignee)

Comment 18

4 years ago
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?
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)
(Assignee)

Comment 20

4 years ago
Created attachment 8557933 [details] [diff] [review]
alternative - set `bidi.direction` when the UI locale is changed

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)
(Assignee)

Comment 21

4 years ago
Created attachment 8557944 [details] [diff] [review]
scrollbar position should follow the document `dir`

(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)
(Assignee)

Comment 23

4 years ago
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+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1115115
Target Milestone: --- → 2.2 S6 (20feb)
Fabien, can we land this?
Flags: needinfo?(fabien)
(Assignee)

Comment 27

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #8557933 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(fabien)
Summary: [RTL][Browser] Scroll bar is on the right side. → [RTL][Browser] Scroll bar is on the wrong side.
(In reply to Michael Henretty [:mhenretty] from comment #30)
> Try link:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=139d21f24b8a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12391199c45f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 34

4 years ago
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)
Duplicate of this bug: 1133102

Updated

4 years ago
Attachment #8557944 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/384ed4b3adcc
status-b2g-v2.2: affected → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
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+

Comment 39

4 years ago
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

Comment 42

4 years ago
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]

Updated

3 years ago
Duplicate of this bug: 1136503
You need to log in before you can comment on or make changes to this bug.