1990292 - 'overflow-x: clip' and 'overflow-y: clip' also cause clipping in the other axis, if the container is transformed

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. See if any lime borders are clipped.

ACTUAL RESULTS:
The second lime rectangle has its left edge clipped.
The third lime rectangle has its right edge clipped.

EXPECTED RESULTS:
No such clipping.

This is the underlying cause of WebCompat bug 1977538.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (flipped the axes, now there's unexpected clipping on top / bottom)

Here's a testcase where I've flipped the axes and we get unexpected clipping on the top of the 2nd lime rect and the bottom of the 3rd lime rect.

Comment 2

[Daniel Holbert [:dholbert]]

The reporter of the webcompat bug observed that we seem to be able to work around this by adjusting the aOutRect.x member to 0 here:
https://searchfox.org/firefox-main/rev/c2e3c0aea49fb45082efdc129e79f84df7a07f48/layout/generic/nsIFrame.cpp#2760-2762,2796-2801,2803

bool nsIFrame::ComputeOverflowClipRectRelativeToSelf(
    const PhysicalAxes aClipAxes, nsRect& aOutRect,
    nsRectCornerRadii& aOutRadii) const {
...
  if (MOZ_UNLIKELY(!aClipAxes.contains(PhysicalAxis::Horizontal))) {
    // NOTE(mats) We shouldn't be clipping at all in this dimension really,
    // but clipping in just one axis isn't supported by our GFX APIs so we
    // clip to our visual overflow rect instead.
    nsRect o = InkOverflowRect();
    aOutRect.x = o.x;
...
  }

That's likely not the right solution exactly, but it's a good clue about where to explore here.

Also, I'll move this to Layout given that that^ code is involved (in ComputeOverflowClipRectRelativeToSelf) which puts this probably more in the layout bucket than the web-painting bucket.

Comment 3

[Daniel Holbert [:dholbert]]

(I suspect we're double-applying the transform to the clip rect, or something like that -- maybe InkOverflowRect() already includes the transform, and then we apply the transform again when aOutRect ultimately gets applied as a clip-rect.)

Comment 4

[Daniel Holbert [:dholbert]]

Perhaps we should be using InkOverflowRectRelativeToSelf() here?
https://searchfox.org/firefox-main/rev/ad97131360471d36dfd9cf85695dde35914843bc/layout/generic/nsIFrame.cpp#8532-8539

nsRect nsIFrame::InkOverflowRectRelativeToSelf() const {
  if (IsTransformed()) {
    if (OverflowAreas* preTransformOverflows =
            GetProperty(PreTransformOverflowAreasProperty())) {
      return preTransformOverflows->InkOverflow();
    }
  }
  return InkOverflowRect();

Looks like it's identical to InkOverflowRect() except for when we're transformed in which case it gets the pre-transformed rect (which should precisely address the thing in comment 3).

Comment 5

[Daniel Holbert [:dholbert]]

Yup, that seems to work. I'll post a patch with that fix and some tests later today.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1990292: Avoid double-applying transforms when setting up clipping rect for 'overflow-{x,y}:clip'. r?#layout

Some context on this patch: The style 'overflow-{x,y}:clip' is only supposed to
clip overflow in the specified axis (x or y). But under the hood, we need to
set up a two-dimensional clipping rect, with some size and position for the
other axis. So we use the ink-overflow rect's size and position, which should be exactly-generous-enough to result in no actual clipping (since it's at the boundary of anything that would be clipped.

However: until this patch, this didn't work quite right in cases where
transforms were involved -- we'd essentially double-transform the rect when
clipping (since the ink-overflow rect already has transforms applied, and then
we transform it when clipping).

This patch fixes that by explicitly using our un-transformed API for
InkOverflow(), so that the transform only gets incorporated once.

Comment 7

[Daniel Holbert [:dholbert]]

./mach try auto: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=154547

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7d38ad46e9d2
https://hg.mozilla.org/integration/autoland/rev/01fb3bd646b6
Avoid double-applying transforms when setting up clipping rect for 'overflow-{x,y}:clip'. r=layout-reviewers,emilio

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/55029 for changes under testing/web-platform/tests

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/01fb3bd646b6

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Addresses a webcompat issue that completely breaks at least one site ( https://app.tangoapp.dev/ ) per https://bugzilla.mozilla.org/show_bug.cgi?id=1977538
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: STR: Load https://bug1990292.bmoattachments.org/attachment.cgi?id=9515078 and make sure that none of the lime rectangles are missing part of their border.

Alternate STR: Visit https://app.tangoapp.dev/welcome and make sure the cookie prompt isn't blank (or missing its text)

• Risk associated with taking this patch: low
• Explanation of risk level: Small/targeted fix, affecting only elements that have a CSS transform and also overflow-x or overflow-y set to clip, fixing an issue that otherwise causes us to misplace the clipping rect.
• String changes made/needed: None
• Is Android affected?: yes

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1990292: Avoid double-applying transforms when setting up clipping rect for 'overflow-{x,y}:clip'. r?#layout

Some context on this patch: the style 'overflow-{x,y}:clip' is only supposed to
clip overflow in the specified axis (x or y). But under the hood, we need to
set up a two-dimensional clipping rect, with some size and position for the
other axis. So we use the ink-overflow rect's size and position, which should
be exactly-generous-enough to result in no actual clipping (since it's at the
boundary of anything that would be clipped).

However: until this patch, this didn't work quite right in cases where
transforms were involved -- we'd essentially double-transform the rect when
clipping (since the ink-overflow rect already has transforms applied, and then
we transform it when clipping).

This patch fixes that by explicitly using our un-transformed API for
InkOverflow(), so that the transform only gets incorporated once.

Original Revision: https://phabricator.services.mozilla.com/D265864

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Phabricator Automation from comment #12)

firefox-beta Uplift Approval Request

• User impact if declined: Addresses a webcompat issue that completely breaks at least one site ( https://app.tangoapp.dev/ ) per https://bugzilla.mozilla.org/show_bug.cgi?id=1977538

(To be fair, I should mention that this TangoApp site doesn't start perfectly-working after this bug is fixed -- the site's core functionality seems to be to use Chromium-only web features to communicate with a USB-connected Android device, and that doesn't work in Firefox for entirely unrelated reasons to this layout bug. But the site has a disclaimer to tell you about that, and you can still navigate around the links on the site etc., once this bug here is fixed.)

Comment 15

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/8290ba5b3e1d
https://hg.mozilla.org/releases/mozilla-beta/rev/fc747ac78913

Comment 16

[Peter Magyari (Desktop QA)]

Verified fixed using Nightly (20250924094045) on Windows 10, MacOS 15.6.1 and Ubuntu 24.04.

Comment 17

[Peter Magyari (Desktop QA)]

Also verified the fix using Beta (20250926090834) on all of the OSes from above.