1527511 - Fix RTL scrollbar rendering at zoom levels greater than 1

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

There is an issue with RTL scrollbar rendering which may have been introduced in part by bug 1423013.

UpdateMinimumScaleSize() is sizing the layout viewport based on the XMost() and YMost() of the scrollable overflow rect, rather than its width and height.

On RTL scrollframes, that can result in a layout viewport that's too small.

With containerless scrolling, this then breaks scrollbar rendering because we apply a clip sized to the layout viewport to the scrollframe contents, and this can clip away the scrollbars. The too-small layout viewport is likely to cause other problems with container scrolling too.

This causes the following tests to fail with containerless scrolling enabled:

layout/reftests/bugs/1133905-4-vh-rtl.html
layout/reftests/bugs/1133905-5-vh-rtl.html
layout/reftests/bugs/1133905-6-vh-rtl.html

(These are the test pages that load at zoom levels greater than 1.)

In bug 1158392 I am going to temporarily disable these tests, so that we can enable containerless scrolling in nightly, but I plan to re-enable in this bug before letting containerless scrolling ride the trains.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix

Hope this patch fixes this issue.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix (with typo fixes)

Of course the previous one doesn't work at all. :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix

I should have waited for compiling.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Note that I think the patch is wrong for the vertical writing mode, we shouldn't factor it into. Probably we should just check IsInlineReversed() there. Anyway I will try the patch after bug 1158392 landed.

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)

With containerless scrolling, this then breaks scrollbar rendering because we apply a clip sized to the layout viewport to the scrollframe contents, and this can clip away the scrollbars.

I don't think this diagnosis is correct. The scroll port clip that's applied to the scrollframe's contents does not apply to the scrollbars.

Comment 6

[Botond Ballo [:botond]]

I debugged this, and the diagnosis from comment 0 was indeed wrong. The issue is unrelated to the scroll port size calculation.

Hiro, if you don't mind I'm going to take over this bug and use it for my fixes for RTL scrollbar rendering.

The scroll port sizing issue is still a bug, and I'll file a follow-up for it.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 9043470 [details] [diff] [review]
A possible fix

(The problem this patch is fixing will be pursued in bug 1534454.)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thanks! I will take a look bug 1534454.

Comment 9

[Botond Ballo [:botond]]

Moving viewport-compat dependency over to bug 1534454.

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1527511 - Scale scroll parts clip by the resolution with containerless scrolling. r=mstange

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1527511 - Get zoomed-in RTL scrollbar rendering tests working and re-enable them. r=kats

Depends on D23077

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1527511 - Other scrollbar rendering reftest fixes. r=kats

The functional fix in this patch series makes some other scrollbar rendering
tests actually test something useful.

Some of them just start passing. Some need a bit of fuzzing. Some need an
adjustment to avoid running afoul of our minimum viewport width of 200.

Depends on D23078

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c2caf4471c6
Scale scroll parts clip by the resolution with containerless scrolling. r=mstange
https://hg.mozilla.org/integration/autoland/rev/bf2716a26aa0
Get zoomed-in RTL scrollbar rendering tests working and re-enable them. r=kats
https://hg.mozilla.org/integration/autoland/rev/1ebd5a9c9c38
Other scrollbar rendering reftest fixes. r=kats

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/5c2caf4471c6
https://hg.mozilla.org/mozilla-central/rev/bf2716a26aa0
https://hg.mozilla.org/mozilla-central/rev/1ebd5a9c9c38

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 9050188 [details]
Bug 1527511 - Scale scroll parts clip by the resolution with containerless scrolling. r=mstange

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1137890
• User impact if declined: On some pages with direction=rtl, scrollbars can be clipped away (rendered incompletely or not at all).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Classifying as medium risk because the fix is touching core layout code which is kind of tricky. However, I am comfortable with uplifting it as this point in the cycle.
• String changes made/needed:
• Do you want to request approval of these patches as well?: on, on

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)

• Do you want to request approval of these patches as well?: on, on

(This feature seems to be broken, but I did intend to request uplift on all three patches.)

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9050188 [details]
Bug 1527511 - Scale scroll parts clip by the resolution with containerless scrolling. r=mstange

Fix for a correctness issue, P3 but the fix has been on nightly for a week with no reported regression and this is impacting containerless scrolling on Android which ship in 67, so uplift approved for 67 beta 8, thanks.

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/31139386e2bd
https://hg.mozilla.org/releases/mozilla-beta/rev/857b245fff74
https://hg.mozilla.org/releases/mozilla-beta/rev/30bd5b7d4bfc

Comment 19

[Botond Ballo [:botond]]

I'm going to mark 66 as unaffected because this is a regression from containerless scrolling which was first enabled in 67.