1932840 - extra scrollable overflow (regression from bug 1768921)

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect)

Description

[Timothy Nikkel (:tnikkel)]

Be on Windows with classic scrollbars.
Goto https://speedometer-preview.netlify.app/?developerMode=
Open the developer panel in the top right.
Unselect all.
Select TodoMVC-Svelte-Complex-DOM
Click Step Test twice. An iframe should load.
Open inspector, ctrl-f for "sprint one". That will find a text control.

Frame tree for that text control
nsTextControlFrame@2c1346315a8 parent=2c134631330 (x=0, y=1056, w=9000, h=1920) [content=2c1308263e0] [cs=2c1341bff08] <
ScrollContainer(div)(-1)@2c1346316a0 parent=2c1346315a8 (x=60, y=60, w=8880, h=1800) [content=2c131fc5e80] [cs=2c134b05f08:-moz-text-control-editing-root] <
Block(div)(-1)@2c134631958 parent=2c1346316a0 (x=0, y=0, w=8880, h=1800) ink-overflow=(x=0, y=0, w=8880, h=1860) scr-overflow=(x=0, y=0, w=8880, h=1860) [content=2c131fc5e80] [cs=2c134623408:-moz-scrolled-content] <
line@2c134631ab8 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,no-break,clear-before:none,clear-after:none(x=660, y=300, w=3831, h=1020) ink-overflow=(x=660, y=240, w=3831, h=1140) scr-overflow=(x=660, y=240, w=3831, h=1140) in-flow-scr-overflow=(x=660, y=240, w=3831, h=1140) <
Text(0)"Sprint one"@2c134631a18 parent=2c134631958 (x=660, y=240, w=3831, h=1140) [content=2c131fc5f00] [cs=2c13463c108:-moz-text] [run=2c1347de100][0,10,T]
>
>
>
>

Before bug 1768921 that text control was not scrollable, but it is now.

(Relevance: this test control being scrolalble means it is the first scrollable thing found so we give it a display port, instead of the sidebar)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1768921

:dshin, since you are the author of the regressor, bug 1768921, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Timothy Nikkel (:tnikkel)]

I built with https://hg.mozilla.org/mozilla-central/rev/3973e4edeb0395914a62704fbf735e1dfd8bd906, so that it includes all of the changesets from bug 1932800, and this problem still happens.

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

I built with https://hg.mozilla.org/mozilla-central/rev/3973e4edeb0395914a62704fbf735e1dfd8bd906, so that it includes all of the changesets from bug 1932800, and this problem still happens.

Same result with https://hg.mozilla.org/mozilla-central/rev/c0c4c089fc10652d4829c9acd7a16e9ff10d776d after bug 1932800 got backed out and relanded.

I think we need to fix this before bug 1768921 can ship.

Comment 4

[Timothy Nikkel (:tnikkel)]

[Tracking Requested - why for this release]: text boxes unexpectedly being scrollable, that also messes up optimizations for finding user scrollable things to set a display port so they are ready to quickly respond to user

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: sv.zip

This is much reduced html file, but still a large css file.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

It might be that we shouldn't create scrollable overflow for padding if overflow-clip-box-inline is content-box, or something along those lines, since in that case it'll always create overflow.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

I'd be surprised if this wasn't an issue on arbitrary text controls, assuming they have padding?

Comment 8

[David Shin[:dshin]]

• The input has a computed height of 32px, with box-sizing: border-box
• padding-top: 5px, padding-bottom: 8px, border-width: 1px so 17px available for content box
• font family ("Segoe UI") and size (14px) selection & results in text frame height of 19px (Text(0)"Sprint one" [...] h=1140)
• Text frame overflows content box by 1px on each side (line[...] (x=660, y=240, w=3831, h=1140))
• Padding inflation of this box causes the overflow height of 31px

Comment 9

[David Shin[:dshin]]

Attachment: A testcase

Padding inflation did bring out the behaviour, but having block-direction for text input seems to be something only we do?
Attached testcase allows block-direction scrolling on Firefox by setting test.scrollTop but not in Safari or Chrome.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1768921

Comment 11

[David Shin[:dshin]]

Attachment: Bug 1932840: Ensure padding inflation does not cause block scroll in single-line input elements. r=#layout

These elements can't be scrolled block-direction by user anyway. It can be
programatically moved, but other browsers don't support that (See bug 1933477).
This also preserve our heuristics for finding user-scrollable elements.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f0c8239c94d
Ensure padding inflation does not cause block scroll in single-line input elements. r=layout-reviewers,emilio

Comment 13

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

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

Comment 14

[Timothy Nikkel (:tnikkel)]

Thanks for working on this!

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1932840 - Minor follow-up to save some content->frame trips. r=dshin

Sorry, my phab suggestion was not fully what I meant. This should be
equivalent.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6f0c8239c94d

Comment 17

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

Upstream PR merged by moz-wptsync-bot

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03ca2fd8e423
Minor follow-up to save some content->frame trips. r=dshin

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:dshin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[David Shin[:dshin]]

Comment on attachment 9440007 [details]
Bug 1932840: Ensure padding inflation does not cause block scroll in single-line input elements. r=#layout

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Prevent potential perf regression
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: (No Nightly today? Fix is manually confirmed on central using below)
  Run WPT test, ensure all passing
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Change is well-constrained, and re-enables optimization lost by bug 1768921
• String changes made/needed:
• Is Android affected?: Yes

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/03ca2fd8e423

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/210bd8510f80

Comment 23

[Daniel Bodea [:danibodea]]

This issue can be reproduced by loading the test in Beta 134.0b3. The fix has been verified in Nightly v135.0a1 and Beta v134.0b4 (try build) in Windows 10. Assuming this is the right verification process, I will close this report. Thank you.