Last Comment Bug 1139306 - [RTL] Scrollbars not rendered correctly when direction:rtl on B2G
: [RTL] Scrollbars not rendered correctly when direction:rtl on B2G
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
P2 normal (vote)
: mozilla40
Assigned To: Simon Montagu :smontagu
:
: Jet Villegas (:jet)
Mentors:
Depends on: 1156918 1177334
Blocks: gaia-rtl
  Show dependency treegraph
 
Reported: 2015-03-04 01:23 PST by Sue
Modified: 2015-06-24 22:57 PDT (History)
22 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard: [rtl-impact][MGSEI-Triage+]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.2+
wontfix
wontfix
fixed
verified
verified


Attachments
video.MP4 (4.47 MB, video/mp4)
2015-03-04 01:23 PST, Sue
no flags Details
v2.2_video.MP4 (2.17 MB, video/mp4)
2015-03-30 19:06 PDT, Sue
no flags Details
frame-display-dump (83.01 KB, text/plain)
2015-03-31 13:37 PDT, (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
POC patch (1.08 KB, patch)
2015-04-14 15:33 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
POC patch (2.07 KB, patch)
2015-04-14 15:34 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch (4.85 KB, patch)
2015-04-15 07:18 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.2, with the extra files from comment 34 (7.88 KB, patch)
2015-04-16 02:56 PDT, Simon Montagu :smontagu
tnikkel: review+
Details | Diff | Splinter Review
Patch v.3 (14.83 KB, patch)
2015-04-22 04:31 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.4 (11.49 KB, patch)
2015-05-01 07:39 PDT, Simon Montagu :smontagu
tnikkel: review+
jocheng: approval‑mozilla‑b2g37+
Details | Diff | Splinter Review
fix up reftests (11.02 KB, patch)
2015-05-07 21:52 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
fix up reftests v2 (15.07 KB, patch)
2015-05-08 11:30 PDT, Timothy Nikkel (:tnikkel)
smontagu: review+
Details | Diff | Splinter Review
Just a little more fuzz (1.27 KB, patch)
2015-05-10 02:23 PDT, Simon Montagu :smontagu
tnikkel: review+
Details | Diff | Splinter Review
verify_v3.0_pass.mp4 (2.30 MB, video/mp4)
2015-05-27 19:13 PDT, Sue
no flags Details
verify_v2.2_pass.png (49.50 KB, image/png)
2015-05-31 20:31 PDT, Sue
no flags Details

Description User image Sue 2015-03-04 01:23:16 PST
Created attachment 8572510 [details]
video.MP4

[1.Description]:
[RTL][Contacts]When scroll down/up the login panel, there is no scroll bar.
See attachment:video.MP4

[2.Testing Steps]: 
Prerequisite:Set system language as Arabic.
1. Launch Contacts app.
2. Tap settings icon at right top.
3. Select Import Contacts -> Gmail.
4. Scroll down and up the log in page.

[3.Expected Result]: 
4. The scroll bar should be left-aligned and can move vertically.

[4.Actual Result]: 
4. The scroll bar disappeared.

[5.Reproduction build]: 
Flame 2.2 build:
Build ID               20150303002527
Gaia Revision          3d188c414e30acc392253d5389a42352fcfbc183
Gaia Date              2015-03-03 00:53:42
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c89aad487aa5
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150303.034757
Firmware Date          Tue Mar  3 03:48:06 EST 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150303010233
Gaia Revision          c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gaia Date              2015-03-02 20:14:21
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/0b3c520002ad
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150303.044015
Firmware Date          Tue Mar  3 04:40:27 EST 2015
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
Free Test

[8.Note]:
When link contacts from Facebook, the scroll bar in log in FB page also disappeared.
Comment 1 User image Johan Lorenzo [:jlorenzo] 2015-03-04 05:44:14 PST
[Blocking Requested - why for this release]: New feature which will impact all the app using window.open()
Comment 2 User image Delphine Lebédel [:delphine - use Need Info] 2015-03-05 14:54:16 PST
Triage: P2
(are these videos always corrupt just for me? :( )
Comment 3 User image Tim Guan-tin Chien [:timdream] (please needinfo) 2015-03-11 19:44:29 PDT
Kaze, you dealt with this issue before. Is this a dup or something require a platform fix so we should kick it off 2.2+?
Comment 4 User image Kai-Chih Hu [:khu] 2015-03-19 02:36:41 PDT
David, would you mind checking with Kaze? 
This ni is pending for a week. Thanks.
Comment 5 User image David Scravaglieri [:scravag] 2015-03-27 16:48:31 PDT
Reassign to comms devs
Comment 6 User image Julien Wajsberg [:julienw] 2015-03-28 02:15:37 PDT
Looks like scrolling stuff.

Kats, maybe you could help sheding some light?
Comment 7 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-03-30 05:07:12 PDT
When I use the "Mirrored english" language on a local build and follow the STR from comment 0 I get a google sign-in page that is LTR and the scrollbar shows up normally on the right side. I tried finding this window in WebIDE so that I can force it to be RTL but it doesn't appear to be accessible in WebIDE. Even if I switch to proper Arabic on a nightly build unless I can inspect this window in WebIDE it's going to be hard to say what's going on.
Comment 8 User image Julien Wajsberg [:julienw] 2015-03-30 09:41:55 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> When I use the "Mirrored english" language on a local build and follow the
> STR from comment 0 I get a google sign-in page that is LTR and the scrollbar
> shows up normally on the right side.

According to comment 0 the scrollbar should be on the left side.


> I tried finding this window in WebIDE
> so that I can force it to be RTL but it doesn't appear to be accessible in
> WebIDE. Even if I switch to proper Arabic on a nightly build unless I can
> inspect this window in WebIDE it's going to be hard to say what's going on.

Kats, in WebIDE, this window lives in the 'System' application and you can select the iframe from the "Iframe selector" (that you need to enable in the Dev tools preferences first).


(In reply to Delphine Lebédel [:delphine - in SA for l10n events until April 7th, expect delay in answering] from comment #2)

> (are these videos always corrupt just for me? :( )

They are not, you need to download and play them locally. They're just not playable inside Firefox (I don't know why).


Sue, can you please create a new video with an uptodate build? The video is nearly 1 month old now...
Comment 9 User image Sue 2015-03-30 19:06:23 PDT
Created attachment 8585837 [details]
v2.2_video.MP4

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Sue, can you please create a new video with an uptodate build? The video is
> nearly 1 month old now...

Hi Julien,
I have uploaded the video with latest build of Flame 2.2, hope that can help.
See attachment:v2.2_video.MP4
Comment 10 User image howie [:howie] 2015-03-31 03:48:44 PDT
Hi Gregor, not sure if this component falls to you?
Comment 11 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-03-31 04:56:11 PDT
(In reply to Julien Wajsberg [:julienw] from comment #8)
> Kats, in WebIDE, this window lives in the 'System' application and you can
> select the iframe from the "Iframe selector" (that you need to enable in the
> Dev tools preferences first).

I tried this but I couldn't see the iframe there. After I follow the STR to get to the google login screen, open the system app in WebIDE, and look at the available frames to debug, there are only two options:
app://system.gaiamobile.org/index.html
app://callscreen.gaiamobile.org/index.html

and neither of these is the correct one. I also tried looking in the Communications app but didn't see the window there - I see index.html, curtain.html, oauth.html, and an about:blank.

This time I was trying on the latest nightly pvtbuild using Arabic as the language - and yes, I can repro the missing scrollbar.
Comment 12 User image Gregor Wagner [:gwagner] 2015-03-31 08:05:46 PDT
Kevin, can you take a look?
Comment 13 User image Julien Wajsberg [:julienw] 2015-03-31 10:14:49 PDT
Kats, I had to ask Alexandre Poirot because I couldn't find it myself :)

So you need to attach to the System app _before_ clicking on "Gmail Import", because the iframe picker doesn't know how to find the existing iframes, it can only know the new ones...

Hope this helps!
Comment 14 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-03-31 13:11:38 PDT
I had to attach to the _Communications_ app before opening the iframe (actually I just started the Communications app directly using WebIDE) and then I was able to find the relevant iframe. And from inspecting in WebIDE all the properties of the document seem ok. I also looked at the layer dump and there's no scrollbars being generated at all, so this is a layout bug. If I toggle the document from rtl to ltr the scrollbars appear as expected.
Comment 15 User image Kevin Grandon :kgrandon 2015-03-31 13:23:59 PDT
Thanks for investigating kats. Clearing my needinfo for now, but ping me if there's anything I can do.
Comment 16 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-03-31 13:37:17 PDT
Created attachment 8586344 [details]
frame-display-dump

Here's a dump of the frame tree and display list from the communications process while the Google login page is displayed in arabic. I might be misreading it but it looks like the vertical scrollbar is positioned just to the left of the visible area (x=-480 app units) and no display item is generated for it.
Comment 17 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-03-31 13:57:16 PDT
Based on discussion on IRC with dbaron and tn, this might be a bug in RTL position of overlay scrollbars. CC'ing :jfkthame and :smontagu.
Comment 18 User image Timothy Nikkel (:tnikkel) 2015-03-31 14:22:54 PDT
I'm not sure if this is the cause but it's a place to look. The code at

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=abea26b5a89f#4967

should be moving the scrollbar from "just off screen to the left" to "on screen, and flush with the left side of the page".
Comment 19 User image Gregor Wagner [:gwagner] 2015-04-07 17:14:16 PDT
Jet, can you help out finding an owner here?
Comment 20 User image Jet Villegas (:jet) 2015-04-09 11:23:46 PDT
I posted a simpler test case here:
http://media.junglecode.net/test/1139306/

Loading that test in the B2G browser shows the problem. It confirms an error in how we layout scrollbars with direction:rtl on B2G

The <iframe> contents are direction:rtl and scrollable. On B2G (simulator and device), the scrollbar is not shown on that iframe in any language set in the preferences. 

The "click me" button opens a new window with the Google login page. Depending on the language preference (which sets google.com page to direction:rtl if Arabic is selected) the scrollbars also disappear.

Simon: can you debug this one?
Comment 21 User image Josh Cheng [:josh] 2015-04-13 17:27:59 PDT
Hi Simon,
Can you help to reply comment 20 from Jet?
Thanks!
Comment 22 User image Simon Montagu :smontagu 2015-04-14 15:08:03 PDT
AFAICT this is a styling issue rather than a layout bug: something is giving scrollbars a negative left margin, which pushes them off the viewport in RTL frames.

I haven't yet found where this margin is set. Changing the margin for scrollbars in b2g/chrome/content/content.css doesn't have any effect.
Comment 23 User image Simon Montagu :smontagu 2015-04-14 15:16:33 PDT
kats, is there a way to find out where a particular style is coming from, like Inspect Element in desktop browser?
Comment 24 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-04-14 15:24:36 PDT
All scrollbar styling should be in b2g/chrome/content/content.css. You can inspect other content styles using WebIDE but I don't think they can style the scrollbars.

To use WebIDE, you'll need to flash the build, go into the developer settings and enable devtools, add |user_pref("devtools.debugger.forbid-certified-apps", false);| to the prefs.js using ./edit-prefs.sh. Then you can connect to the device with WebIDE and see comment 14 for the rest on how to inspect the iframe. I can provide more details on that if you get stuck.
Comment 25 User image Simon Montagu :smontagu 2015-04-14 15:33:12 PDT
Created attachment 8592503 [details] [diff] [review]
POC patch

This patch works around the bug by mirroring the margins on RTL scrollbar frames. I'm not suggesting we should check this in, but it spotlights the underlying issue.
Comment 26 User image Simon Montagu :smontagu 2015-04-14 15:34:55 PDT
Created attachment 8592506 [details] [diff] [review]
POC patch
Comment 27 User image Timothy Nikkel (:tnikkel) 2015-04-14 15:52:44 PDT
The negative margins in content.css are needed to give the scrollbars a non-zero width. Without the negative margins they are 0 width. (That explains why removing them doesn'The -moz-margin-start should ideally get translated into a right margin in an RTL scrollframe. But that doesn't seem to be happening. I can't figure out why because I can't find where -moz-margin-start gets computed in the style system.

If we have to go with the workaround route we should put it in nsScrollbarFrame::GetMargin as that gets the scrollbar size from the theme if available (b2g doesn't have a theme, but mac uses this for example).
Comment 28 User image Simon Montagu :smontagu 2015-04-14 16:25:16 PDT
(In reply to Timothy Nikkel (:tn) from comment #27)
> The negative margins in content.css are needed to give the scrollbars a
> non-zero width. Without the negative margins they are 0 width. (That
> explains why removing them doesn'The -moz-margin-start should ideally get
> translated into a right margin in an RTL scrollframe. But that doesn't seem
> to be happening.

I tried removing them, putting in different values, setting margin-left and -right instead of -moz-margin-start. Whatever I do the right margin stays at zero and the left margin stays at 960 app pixels (in the emulator). From this I concluded that the setting is coming from somewhere else and not from content.css.
Comment 29 User image Timothy Nikkel (:tnikkel) 2015-04-14 16:39:44 PDT
I changed content.css only and it changed the margin values I was seeing on my flame. There are two moz margin start's. I removed the second and set mix margin start to unset in the first and set margin right to the negative value. This made the scroll bar show in jets test case.
Comment 30 User image Simon Montagu :smontagu 2015-04-14 17:08:14 PDT
So from content 27 and 29 and my debugging, it looks as if the emulator on mac is overriding the margin from content.css with data from the theme.
Comment 31 User image Simon Montagu :smontagu 2015-04-14 17:42:16 PDT
Neither setting -moz-margin-start in content.css nor using a selector with [dir="rtl"] is not quite right, because the position of the scrollbar is determined by the layout.scrollbar.side pref, not necessarily by the direction of the content, nor by the direction of the scrollbar itself or any of its ancestors.

I think that we do need to pass scrollbarOnLeft into nsScrollbarFrame::GetMargin and swap left and right margins accordingly. I'll work on a patch for that tomorrow.
Comment 32 User image Simon Montagu :smontagu 2015-04-15 07:18:48 PDT
Created attachment 8592819 [details] [diff] [review]
Patch

I think it's preferable to add a helper method to swap the left and right margins in the scrollbar-on-left case, like so, rather than adding a boolean argument to nsIFrame::GetMargin which will mostly not be used.
Comment 34 User image Timothy Nikkel (:tnikkel) 2015-04-15 10:46:32 PDT
Comment on attachment 8592819 [details] [diff] [review]
Patch

We'll also need to change:
browser/themes/linux/devtools/floating-scrollbars.css
browser/themes/osx/devtools/floating-scrollbars.css
browser/themes/windows/devtools/floating-scrollbars.css
mobile/android/themes/core/content.css

Do you know why -moz-margin-start isn't getting translated to a right margin here?
Comment 35 User image Simon Montagu :smontagu 2015-04-16 01:50:06 PDT
(In reply to Timothy Nikkel (:tn) from comment #34)
> Do you know why -moz-margin-start isn't getting translated to a right margin
> here?

I assume because scrollbars don't inherit direction.
Comment 36 User image Simon Montagu :smontagu 2015-04-16 02:56:09 PDT
Created attachment 8593287 [details] [diff] [review]
Patch v.2, with the extra files from comment 34

FTR, viewing an RTL page in Responsive Design View has exactly the same bug, and this version of the patch fixes it there.
Comment 37 User image Simon Montagu :smontagu 2015-04-16 03:10:51 PDT
(In reply to Simon Montagu :smontagu from comment #35)
> (In reply to Timothy Nikkel (:tn) from comment #34)
> > Do you know why -moz-margin-start isn't getting translated to a right margin
> > here?
> 
> I assume because scrollbars don't inherit direction.

No, that's wrong: in a version of Jet's testcase with <body dir="rtl">, the RTL textarea has a scrollbar and the LTR textarea does not. Apparently the problem is rather that the scrollbar is not a descendant of the element whose direction is set.
Comment 38 User image Timothy Nikkel (:tnikkel) 2015-04-16 09:54:45 PDT
Comment on attachment 8593287 [details] [diff] [review]
Patch v.2, with the extra files from comment 34

Thanks. A couple nits.

>+/* Scrollbar code will reset the margin to the correct side depending on
>+   layout.scrollbar.side pref */
> xul|scrollbar[orient="vertical"] {
>-  -moz-margin-start: -8px;
>+  margin-left: -8px;

How about "Scrollbar code will reset the margin to the correct side depending on where layout actually puts the scrollbar"?

Can we just incorporate the logic of GetBidiMirroredMargin into nsScrollbarFrame::GetMargin? That way we prevent someone from using the wrong margin by forgetting to call GetBidiMirroredMargin. Change nsScrollbarFrame::GetMargin to nsScrollbarFrame::GetMarginForScrollbar (better name?) and pass the scrollbar on left bool to it. It should be fine not to override nsIFrame::GetMargin.
Comment 40 User image Simon Montagu :smontagu 2015-04-21 01:37:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3337ed60a1 With the nits from comment 38, plus some knock-on changes because I wanted to pass an enum instead of a bool.
Comment 41 User image Carsten Book [:Tomcat] 2015-04-21 06:20:02 PDT
https://hg.mozilla.org/mozilla-central/rev/3c3337ed60a1
Comment 42 User image Timothy Nikkel (:tnikkel) 2015-04-21 13:12:40 PDT
Backed this out from inbound because of bug 1156918
https://hg.mozilla.org/integration/mozilla-inbound/rev/799784735f9e
Comment 43 User image Simon Montagu :smontagu 2015-04-21 16:29:38 PDT
It turns out that changing nsScrollbarFrame::GetMargin into something that doesn't override nsIFrame::GetMargin causes problems. We need to either bring back the override + helper function to swap the margins, or to make a parallel change to nsBox::AddMargin (which calls GetMargin)
Comment 44 User image Simon Montagu :smontagu 2015-04-22 04:31:48 PDT
Created attachment 8595917 [details] [diff] [review]
Patch v.3

I think that this way is the better option, with GetScrollbarMargin as a helper and keeping nsScrollbarFrame::GetMargin overriding nsIFrame::GetMargin. WDYT?
Comment 45 User image Simon Montagu :smontagu 2015-04-29 00:37:54 PDT
review ping?
Comment 46 User image Timothy Nikkel (:tnikkel) 2015-04-30 15:00:14 PDT
Comment on attachment 8595917 [details] [diff] [review]
Patch v.3

This is good. Sorry for steering you wrong and sorry for the delay.

What do you think about asking the parent of the nsScrollbarFrame (it should be a scrollframe) in GarMargin which side the scrollbar is on and then just returning the correct value from nsScrollbarFrame::GetMargin so we don't need GetScrollbarMargin? That way GetMargin is always returning the correct values.
Comment 47 User image Simon Montagu :smontagu 2015-05-01 07:39:55 PDT
Created attachment 8600316 [details] [diff] [review]
Patch v.4

I think that's a great idea, but I found the hard way that it's a little bit more complicated: the parent of the nsScrollbarFrame isn't always a scrollframe, for example in XUL trees. 

I think that what we need to do is call GetScrollbarMediator, which in turn means that we need to move IsScrollbarOnRight up to nsIScrollbarMediator, but that's OK.

So far this seems to work well, but I want to do some more testing before requesting review.
Comment 48 User image Timothy Nikkel (:tnikkel) 2015-05-06 09:31:43 PDT
Gently tagging needinfo so this doesn't fall through the cracks. :)
Comment 49 User image Simon Montagu :smontagu 2015-05-07 04:40:52 PDT
A 'last' try run before checkin at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8f4ff5aafa8 regressed all the vertical rtl testcases from bug 1133905 on B2G.

Since the scroll bars won't have been visible before this checkin anyway, it's not easy to figure what the regression was.
Comment 50 User image Simon Montagu :smontagu 2015-05-07 04:57:35 PDT
That said, I see that the failures are much the same as the B2G fuzzy-ifs you already have on some of those tests. Shall I just add more of the same?
Comment 51 User image Jonathan Kew (:jfkthame) 2015-05-07 05:14:06 PDT
It looks like the length of the scrollbar "thumb" is varying, depending on the viewport width, yet they're all being compared to the same reference case.

What seems odd is that this is apparently not happening with the LTR versions of the same tests.

So how (if at all) *should* the scroll bars be affected by the viewport width? Should this result in the document loading with a different zoom factor? In that case it would make sense that the length of the thumb would vary across these testcases, and it's wrong to compare them all to the same reference. But if that's so, why don't we get the same failures in the LTR tests?

Clearly I don't really understand this, but it feels to me like something's wrong at some level...
Comment 52 User image Simon Montagu :smontagu 2015-05-07 05:25:14 PDT
Note that we do get failures in the equivalent LTR tests numbered 5 and 6. Perhaps the problem only kicks in on LTR above a certain width for some reason?
Comment 53 User image Timothy Nikkel (:tnikkel) 2015-05-07 12:11:26 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #51)
> It looks like the length of the scrollbar "thumb" is varying, depending on
> the viewport width, yet they're all being compared to the same reference
> case.
> 
> What seems odd is that this is apparently not happening with the LTR
> versions of the same tests.
> 
> So how (if at all) *should* the scroll bars be affected by the viewport
> width? Should this result in the document loading with a different zoom
> factor? In that case it would make sense that the length of the thumb would
> vary across these testcases, and it's wrong to compare them all to the same
> reference. But if that's so, why don't we get the same failures in the LTR
> tests?
> 
> Clearly I don't really understand this, but it feels to me like something's
> wrong at some level...

You are quite right in your comment. The reason the LTR tests pass is because the scrollbars aren't visible at all. Except as Simon notes in comment 52 after we reach a certain width.

When writing tests for bug 1133905 I realized that scrollbars are not placed the same way in b2g reftests as they are when using a b2g device for some reason. There is also an extra scrollbar in the snapshots that I'm not sure where it is coming from. So the reftests for bug 1133905 are very far from ideal, but they seemed like the only option (mochitest, and mochitest-chrome tests did not work at all to test scrollbar positioning code on b2g). And I knew that I would have to deal with scrollbars again to land bug 1153589.

The good news is that with this patch it looks like the RTL tests from bug 1133905 are now working as I expected. I planned to vary the total height of the page as the viewport width varied so that the overall size of the scrollbar thumbs was as close to equal as possible, and then just as a small amount of fuzz. I'll push some changes to try to get these changes made.
Comment 54 User image Timothy Nikkel (:tnikkel) 2015-05-07 21:52:32 PDT
Created attachment 8603156 [details] [diff] [review]
fix up reftests

This should adjust those reftests so they pass after your patch.
Comment 55 User image Simon Montagu :smontagu 2015-05-08 02:38:37 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04b06dcfc1c is a try push with my patch plus tn's reftest fix. It seems to cause regressions on other platforms :(
Comment 56 User image Jonathan Kew (:jfkthame) 2015-05-08 03:10:34 PDT
(In reply to Simon Montagu :smontagu from comment #55)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e04b06dcfc1c is a
> try push with my patch plus tn's reftest fix. It seems to cause regressions
> on other platforms :(

It would, because other platforms aren't affected by the <meta viewport>.

Forcing the pref dom.meta-viewport.enabled to true might fix this?
Comment 57 User image Timothy Nikkel (:tnikkel) 2015-05-08 11:30:36 PDT
Created attachment 8603470 [details] [diff] [review]
fix up reftests v2

Sorry about that. I don't think the tests make much sense if we run them without an async pan zoom controller to interpret the viewport tag. So I just made them skip-if(!asyncPanZoom). I think that should be the right condition on which to run these tests.
Comment 58 User image Simon Montagu :smontagu 2015-05-10 02:23:33 PDT
Created attachment 8603809 [details] [diff] [review]
Just a little more fuzz

Almost there... there is just one more failure on the last couple of try pushes, and it seems to be just the same as the cases that were already fuzzed in bug 1148889 attachment 8589707 [details] [diff] [review]
Comment 59 User image Timothy Nikkel (:tnikkel) 2015-05-11 11:52:40 PDT
Comment on attachment 8603809 [details] [diff] [review]
Just a little more fuzz

Weird that a scrollbar patch would affect printing reftests on android. Android draws it's own scrollbars and ignore the layout ones. But the difference is so tiny I don't think it's worth blocking this.
Comment 60 User image (away until Feb21) Kartikaya Gupta (email:kats@mozilla.com) 2015-05-11 12:06:32 PDT
Android reftests are crazy, so I would disregard any android reftest failures and just skip/fuzz as needed. See bug 1156817 for details.
Comment 63 User image Ryan VanderMeulen [:RyanVM] 2015-05-26 13:21:26 PDT
Please request b2g37 approval on these patches when you get a chance.
Comment 64 User image Simon Montagu :smontagu 2015-05-27 03:06:12 PDT
Comment on attachment 8600316 [details] [diff] [review]
Patch v.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): it's hard to point to a specific cause; it's a combination of using overlay scrollbars and styling scrollbars for RTL 
User impact if declined: no scrollbar for RTL users.
Testing completed: Tested on desktop and B2G builds in RTL and LTR languages, baked on trunk since 2015-05-12
Risk to taking this patch (and alternatives if risky): there could be regressions in scrollbar appearance in some obscure untested combination of options.
String or UUID changes made by this patch: none
Comment 65 User image Josh Cheng [:josh] 2015-05-27 07:32:40 PDT
Hi Sue, Hi Norry,
Please verify on master before approving for 2.2.
Please also perform some regression on RTL and LTR to ensure no regression.
Thanks
Comment 66 User image Sue 2015-05-27 19:13:59 PDT
Created attachment 8611618 [details]
verify_v3.0_pass.mp4

This issue has been verified as pass on latest build of Flame 3.0 and Nexus 5 3.0 by STRs in comment 0.
Results: The scroll bar is left-aligned and can move vertically in RTL.
See attachment:verify_v3.0_pass.mp4
Rate:0/3

Device: Flame 3.0 (pass)
Build ID               20150527160204
Gaia Revision          05380df3158fa39e1dde1687c0bf11a71f8c6868
Gaia Date              2015-05-27 06:27:27
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2c815cc65cc9
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150527.193645
Firmware Date          Wed May 27 19:36:54 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 3.0 (pass)
Build ID               20150527160204
Gaia Revision          05380df3158fa39e1dde1687c0bf11a71f8c6868
Gaia Date              2015-05-27 06:27:27
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2c815cc65cc9
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150527.193206
Firmware Date          Wed May 27 19:32:20 EDT 2015
Bootloader             HHZ12f
Comment 67 User image Ryan VanderMeulen [:RyanVM] 2015-05-28 10:40:36 PDT
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9b84135f5fb5
Comment 68 User image Sue 2015-05-31 20:31:04 PDT
Created attachment 8613351 [details]
verify_v2.2_pass.png

This issue has been verified as pass on latest build of Flame 2.2 and Nexus5 2.2 by STRs in comment 0.
Result: The scroll bar is left-aligned and can move vertically in RTL.
See attachment:verify_v2.2_pass.png
Rate:0/5

Device: Flame 2.2 (pass)
Build ID               20150531162502
Gaia Revision          b4582cc394e0919623263997c0cdb0b4751a1403
Gaia Date              2015-05-31 11:06:34
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/78d8b0a4303d
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150531.195816
Firmware Date          Sun May 31 19:58:28 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (pass)
Build ID               20150531002502
Gaia Revision          0a46394dbee0ed2eb71a136cee38ddd8549dd597
Gaia Date              2015-05-30 14:50:16
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ed2f6aeb1d81
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150531.043812
Firmware Date          Sun May 31 04:38:27 EDT 2015
Bootloader             HHZ12f

Note You need to log in before you can comment on or make changes to this bug.