Closed Bug 1527511 Opened 9 months ago Closed 8 months ago

Fix RTL scrollbar rendering at zoom levels greater than 1

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Keywords: correctness, regression)

Attachments

(3 files, 3 obsolete files)

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.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Attached file A possible fix (obsolete) —

Hope this patch fixes this issue.

Attached file A possible fix (with typo fixes) (obsolete) —

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

Attachment #9043468 - Attachment is obsolete: true
Attached patch A possible fix (obsolete) — Splinter Review

I should have waited for compiling.

Attachment #9043469 - Attachment is obsolete: true
Attachment #9043470 - Attachment is patch: true
Attachment #9043470 - Attachment mime type: application/octet-stream → text/plain

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.

Flags: needinfo?(hikezoe)

(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.

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.

Assignee: hikezoe → botond
Comment on attachment 9043470 [details] [diff] [review]
A possible fix

(The problem this patch is fixing will be pursued in bug 1534454.)
Attachment #9043470 - Attachment is obsolete: true
Flags: needinfo?(hikezoe)

Thanks! I will take a look bug 1534454.

Moving viewport-compat dependency over to bug 1534454.

No longer blocks: viewport-compat

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

Pushed by bballo@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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
Attachment #9050188 - Flags: approval-mozilla-beta?

(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 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.

Attachment #9050188 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Type: enhancement → defect
You need to log in before you can comment on or make changes to this bug.